contextify: share security token with debug ctx #748
Conversation
If security token does not match - no changes are allowed to the objects from different context. We already copy the security token to the newly created contexts in contextify.cc . Copy the security token to the debug context too. Fix: nodejs/node-v0.x-archive#9156
|
cc @iojs/collaborators @domenic |
|
Same PR for node.js: nodejs/node-v0.x-archive#9157 |
| @@ -236,7 +236,13 @@ class ContextifyContext { | |||
| Local<String> script_source(args[0]->ToString(args.GetIsolate())); | |||
| if (script_source.IsEmpty()) | |||
| return; // Exception pending. | |||
| Context::Scope context_scope(Debug::GetDebugContext()); | |||
|
|
|||
| Environment* env = Environment::GetCurrent(args.GetIsolate()); | |||
There was a problem hiding this comment.
Could move this into the if, right?
There was a problem hiding this comment.
It will account for 0% of cases, because debug context is empty only when we are hard on memory (i.e. allocation failure).
There was a problem hiding this comment.
Actually, I meant to say that it'll be executed in 99.999% of cases anyway, not 0%.
There was a problem hiding this comment.
Yeah just thinking about keeping variables in their tightest scope. No big deal.
|
Needs a test |
|
@domenic ! forgot to push a test, sorry. |
|
CI is happy with the change |
| assert.equal(proto.prop, true); | ||
| proto = vm.runInDebugContext('DebugCommandProcessor.prototype'); | ||
| assert.equal(proto.prop, true); | ||
| proto = vm.runInDebugContext('DebugCommandProcessor.prototype.prop = true;' + |
There was a problem hiding this comment.
Should this line be setting to a different value than true? To be sure it actually does something?
There was a problem hiding this comment.
I think this line could be removed.
There was a problem hiding this comment.
My understanding of this line was it was testing that if you set it inside the vm, instead of outside, it still works. So it might be good to leave it in but use a different value.
|
Wait... This does not fix the issue. Oh gosh. |
|
@3y3 I hope you don't mind continuing the discussion here. I'm just tired of updating this issue in both places :) I'm afraid that issue might be a Submitted v8 issue https://code.google.com/p/v8/issues/detail?id=3876&thanks=3876&ts=1423353590 . Going to close this PR for now. |
If security token does not match - no changes are allowed to the objects
from different context. We already copy the security token to the newly
created contexts in contextify.cc . Copy the security token to the debug
context too.
Fix: nodejs/node-v0.x-archive#9156