Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2040053003:
Use GetDebuggedContext instead of GetCallingContext in v8_inspector (Closed)
|
Created:
4 years, 6 months ago by jochen (gone - plz use gerrit) Modified:
4 years, 5 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse GetDebuggedContext instead of GetCallingContext in v8_inspector
R=haraken@chromium.org
BUG=
Patch Set 1 #
Total comments: 7
Messages
Total messages: 16 (3 generated)
LGTM https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/InjectedScript.cpp (right): https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/InjectedScript.cpp:276: v8::Local<v8::Context> callingContext; callingContext => debuggerContext ? https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp (right): https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp:77: v8::Local<v8::Context> callingContext; Ditto.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
not lgtm. v8_inspector needs to be v8 5.0 backwards compatible until either it gets merged into node v6 or into v8 itself. You can use V8_MAJOR_VERSION / V8_MINOR_VERSION to guard changes if they can't wait.
https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp (right): https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp:78: if (!v8::Debug::GetDebuggedContext(isolate()).ToLocal(&callingContext)) We should not need calling context here - injected script is created in the controlled code path, so we should be able to enter the context.
https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp (right): https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp:78: if (!v8::Debug::GetDebuggedContext(isolate()).ToLocal(&callingContext)) On 2016/06/07 at 14:39:29, pfeldman wrote: > We should not need calling context here - injected script is created in the controlled code path, so we should be able to enter the context. so just delete this check?
Description was changed from ========== Use GetDebuggedContext instead of GetCallingContext in v8_inspector R=haraken@chromium.org BUG= ========== to ========== Use GetDebuggedContext instead of GetCallingContext in v8_inspector R=haraken@chromium.org BUG= ==========
pfeldman@chromium.org changed reviewers: + dgozman@chromium.org, kozyatinskiy@chromium.org
> so just delete this check? +dgozman, kozyatinskiy We used to have explicit contract for the methods that can end up creating injected script vs the ones looking it up, but it seems like things are now lazy and anything can create it. I would need to go through the usages and bring sanity back to be sure, but otoh we only create injected script from inspector code, we know the context we create it for, so I don't see why we would need the check.
what about I float my API addition to node, and then we just change the inspector to use it?
https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp (right): https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp:78: if (!v8::Debug::GetDebuggedContext(isolate()).ToLocal(&callingContext)) On 2016/06/07 14:52:13, jochen wrote: > On 2016/06/07 at 14:39:29, pfeldman wrote: > > We should not need calling context here - injected script is created in the > controlled code path, so we should be able to enter the context. > > so just delete this check? I'm worried here about V8DebuggerAgent::currentCallFrames. In this call we create related InjectedScript for each call frame in current stack on pause and callingContext is top callFrame, targetContext is frame context. But if we are able to call function from target context then it will be safe to create InjectedScript in target context. If this case is safe then we can remove this check.
https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/InjectedScript.cpp (right): https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/InjectedScript.cpp:277: if (!v8::Debug::GetDebuggedContext(m_context->isolate()).ToLocal(&callingContext)) Note that GetDebuggedContext says "while in debug context", which is not always the case here - we may wrap some objects (e.g. DOM nodes) when not entered into debugger context. I don't see how implementation of GetDebuggedContext uses that fact though. Should we change the comment? https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp (right): https://codereview.chromium.org/2040053003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/InspectedContext.cpp:78: if (!v8::Debug::GetDebuggedContext(isolate()).ToLocal(&callingContext)) I think we'd better keep it for this call: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/v8_in... This access checks are sanity checks anyway, since they originate from native calls, and we only check the calling context, not the object being wrapped "belonging" to the InjectedScript's context.
well, the problem is that "calling context" is not what you think it does, e.g., when a tail call is used, the context isn't pushed on the stack. I assumed that we're always in the debug context here, at least that's what happened in the layout tests. If that's not the case, we should pass through the context instead of relying on whatever happens to be on the stack
> I assumed that we're always in the debug context here, at least that's what > happened in the layout tests. I now need to go back in time and remember what calling context means and how we rely on it :( Should we sync up on/after Blnkon?
On 2016/06/09 at 19:01:51, pfeldman wrote: > > I assumed that we're always in the debug context here, at least that's what > > happened in the layout tests. > > I now need to go back in time and remember what calling context means and how we rely on it :( Should we sync up on/after Blnkon? sgtm
On 2016/06/09 19:28:25, jochen wrote: > On 2016/06/09 at 19:01:51, pfeldman wrote: > > > I assumed that we're always in the debug context here, at least that's what > > > happened in the layout tests. > > > > I now need to go back in time and remember what calling context means and how > we rely on it :( Should we sync up on/after Blnkon? > > sgtm What's the status of this one? Should we introduce an API that looks up the current context skipping the debugger context? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
