Use V8 completion callback API to assert that V8RecursionScope is on the stack whenever invoking script
Created attachment 128004 [details] Patch
Note: this probably can't be landed until http://code.google.com/p/chromium/issues/detail?id=105962 is resolved, but I'd like to get some feedback on the approach here (particularly for the inspector changes).
Comment on attachment 128004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128004&action=review > Source/WebCore/bindings/v8/V8RecursionScope.h:73 > + class NoPageAuthorScriptWillRun { I haven't tracked down all these NDEBUG statements, but it would be good to assert that we actually don't run any author scripts.
This approach looks pretty reasonable. It's going to be tricky to aways get the "page author" bit right, but I think we can manage it.
Comment on attachment 128004 [details] Patch Attachment 128004 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11560280
(In reply to comment #3) > (From update of attachment 128004 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128004&action=review > > > Source/WebCore/bindings/v8/V8RecursionScope.h:73 > > + class NoPageAuthorScriptWillRun { > > I haven't tracked down all these NDEBUG statements, but it would be good to assert that we actually don't run any author scripts. How might one identify author scripts separately from, say, inspector scripts? The current code assumes that any particular invocation is either one or the other based on its place in the C++ code. But I'd much prefer something more reliable.
Hum... Maybe if you try to create a page author scope inside of a non-page author scope, that should fire an ASSERT?
My though is that this approach seems somewhat error-prone, and I'm looking for ways to help us not screw this bit up in the future. If there isn't a way, that's ok too but it just means we need to worry about this mechanism more in the future.
(In reply to comment #8) > My though is that this approach seems somewhat error-prone, and I'm looking for ways to help us not screw this bit up in the future. If there isn't a way, that's ok too but it just means we need to worry about this mechanism more in the future. Yeah, the trouble is that it itself is trying to fix an error-prone situation (where someone calls out to author script without instantiating a V8RecursionScope). Currently, there don't seem to be any such calls, but the purpose of this code would be to have something besides code review preventing new calls from appearing.
Created attachment 128778 [details] Fix release build
Pavel, I'd be interested on your thoughts on the changes in ScriptDebugServer.cpp. Does this make any sense to you?
Created attachment 133957 [details] Patch
Hey all, I've picked up this patch from Adam & added a bit. I'd like a pre-review from abarth & fishd. Backing up a bit: the idea here is that there are basically three kinds of script invocations that C++ makes into renderer script contexts. 1) Calling "author" script from a frame 2) Calling "author" script from web workers 3) Calling "internal" script (non-page script -- this is generally setup/teardown work that happens in JS). With (1), we want to make sure that inspector before/after work is done (instrumenting, etc...) With (1) & (2) , we need look for end-of-microtask work to do. With (3), we *don't* want to do any supplementary work. The goal of this patch is require every script invocation to declare its intent (either by calling (effectively through V8Proxy to invoke page script) or by stack allocating an object. If a script invocation is added with one of these, we'd like a debug ASSERT to fail. No, this isn't perfect. These "annotations" aren't asserting anything, they are declaring usage type. If someone uses the wrong type, we won't get a warning. The goal is just to get anyone adding new invocations to think about what type it is. Does this make sense? ---- Questions: -The extension's system needs to invoke script callbacks of type (1). I've added a new call to WebFrame for this, but it's a bit strange because it bypasses the frame's scriptController. What's a better way to do this? -The WebScriptScope::BypassFinalizationPhase glue object seems a tad unfortunate. Is there a cleaner way to do this?
Also, here's the beginnings of a patch of what this would look like from the chromium renderer side: https://chromiumcodereview.appspot.com/9865015
> I'd like a pre-review from abarth & fishd. If I don't get to this tomorrow morning, please feel free to remind me. I'm too tired to think about it clearly at the moment.
Sorry for making this more confusing. The sentence above should have read: "If a script invocation is added WITHOUT one of these, we'd like a debug ASSERT to fail.."
Comment on attachment 133957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133957&action=review > Source/WebCore/bindings/v8/DateExtension.cpp:93 > + V8RecursionScope::BypassFinalizationPhase scope; What does BypassFinalizationPhase mean? Maybe V8RecursionScope::InternalScript (to borrow a name from Comment #13 )? > Source/WebKit/chromium/public/platform/WebScriptScope.h:36 > +namespace WebScriptScope { This shouldn't be in platform. It should be up one level in the directory structure. Platform doesn't have any notion of script. > Source/WebKit/chromium/public/platform/WebScriptScope.h:38 > +class BypassFinalizationPhase { I'm worried that clients of this API won't understand how to use it correctly. Why does the embedder know about "finalization phases" or "internal scripts"? Is this required whenever you call into V8, or only when you call via WebFrame? Perhaps and enum parameter on executeScriptAndReturnValue and friends would be better? It depends if you can name things in a way that makes sense to the embedder.
From https://chromiumcodereview.appspot.com/9865015/ it looks like you need to use the recursion scope class whenever you call into V8. Does this include calls into V8 from the network stack? What about in single-process mode?
It's unfortunate that everyone who needs to use the V8 API needs to also talk to WebKit. Is there no way to keep this state in V8 so that folks can interact with V8 without needing to interact with WebKit?
Created attachment 134169 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
> It's unfortunate that everyone who needs to use the V8 API needs to also talk to WebKit. rafaelw and I discussed this aspect of the change. This only affects folks who interact with v8::Contexts created by WebKit. It seems reasonably (although perhaps not ideal) that folks who interact with v8::Contexts created by WebKit need to interact further with WebKit when using those contexts.
Ok. This is ready for a real review. Note a few things: The assert in WebKit::assertV8RecursionScope isn't enabled, as this patch is the first step of a three sided patch. Step two will be landing proper chromium usage of WebScriptInvocation & calling through WebFrame::callFunctionAndReturnValue for extension API invocations. Also, ScriptController:callFunction() doesn't perform the canExecuteScript check as this would break extension apis who are allowed to make these calls even if (for instance) iframe sandboxing is requested. Also, there's an outstanding bug (crbug.com/112892) that an extension context which is paused on a breakpoint may interact badly with an extension event or API callback. This needs further investigation as to the proper solution -- and this patch makes the situation no worse.
Comment on attachment 134169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134169&action=review So, I have some really minor comments below. The only one that's really of any consequence is about ScriptController::callFunction (and the patching WebKit API) being a trap in that it doesn't check canExecuteScripts. As written, this seems likely to lead to bugs where we execute JavaScript even if JavaScript is supposed to be disabled. We probably just need a better name. Although I'm marking this patch R-, please feel free to land everything in here except the WebKit API changes and ScriptController::callFunction with r=me. If you'd like to do that, that will simplify the remaining patch. (You should also feel free to continue to iterate on the full patch if that's easier for you.) > Source/WebCore/bindings/v8/ScriptController.cpp:168 > +ScriptValue ScriptController::callFunction(v8::Handle<v8::Function> function, > + v8::Handle<v8::Object> receiver, > + int argc, > + v8::Handle<v8::Value> argv[]) You can just combine this into one line. > Source/WebCore/bindings/v8/ScriptController.cpp:171 > + // FIXME: This should probably perform the same checks to canExecuteScripts & isPaused that happen in ScriptController::executeScript. > + return ScriptValue(m_proxy->callFunction(function, receiver, argc, argv)); In the bug you said that we shouldn't do that because extensions want to execute script in sandboxed iframes. Should we name this function something that indicates that we're skilling the canExecuteScripts check? callFunctionEvenIfScriptsAreDisabled? > Source/WebCore/bindings/v8/ScriptFunctionCall.cpp:136 > + V8RecursionScope recursionScope(getScriptExecutionContext()); getScriptExecutionContext() should be called scriptExecutionContext(), but that's a problem for a future patch to solve. > Source/WebCore/bindings/v8/V8RecursionScope.h:40 > +// C++ Calls into script contexts which are "owned" by WebKit (initialized in Calls -> calls > Source/WebCore/bindings/v8/V8RecursionScope.h:41 > +// WebKit.cpp) must declare their type: So, this comment isn't quite right. The script contexts (i.e., v8::Context) aren't initialized in WebKit.cpp. The whole V8 engine is initialized there. That means this change affects V8 for the whole process. I still think that's ok, but we should clarify the comment. > Source/WebKit/chromium/public/WebFrame.h:284 > + // Call the function with the given receiver and arguments. > + virtual v8::Handle<v8::Value> callFunctionAndReturnValue( > + v8::Handle<v8::Function>, > + v8::Handle<v8::Object>, > + int argc, > + v8::Handle<v8::Value> argv[]) = 0; It's really unclear that this function skips the canExecuteScripts check, unlike executeScriptAndReturnValue, which seems very similar. If we find a good name for the ScriptController method, that will likely guide us towards a good name here too. > Source/WebKit/chromium/public/WebScriptInvocation.h:2 > + * Copyright (C) 2009 Google Inc. All rights reserved. 2009 -> 2012 > Source/WebKit/chromium/public/WebScriptInvocation.h:36 > +namespace WebScriptInvocation { Is this a common pattern in the WebKit API? I'm still learning about the idioms of the WebKit API, so we might want to double-check with fishd about what idiom he'd recommend here. > Source/WebKit/chromium/public/WebScriptInvocation.h:46 > + Impl* m_impl; Can we use a WebPrivateOwnPtr to avoid calling delete manually? > Source/WebKit/chromium/src/WebFrameImpl.cpp:901 > + v8::Handle<v8::Object> receiver, > + int argc, > + v8::Handle<v8::Value> argv[]) Bad indent. > Source/WebKit/chromium/src/WebScriptInvocation.cpp:48 > +#ifndef NDEBUG > +class BypassMicrotaskCheckpoint::Impl { > +public: > + Impl() { } > + ~Impl() { } > + WebCore::V8RecursionScope::BypassMicrotaskCheckpoint m_scope; > +}; > +#endif Why not just have BypassMicrotaskCheckpoint::Impl inherit from WebCore::V8RecursionScope::BypassMicrotaskCheckpoint ?
Created attachment 134466 [details] Patch
Created attachment 134467 [details] Patch
Comment on attachment 134467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134467&action=review > Source/WebKit/chromium/public/WebFrame.h:280 > + // Call the function with the given receiver and arguments, bypassing > + // canExecut() canExecut() <-- Looks like the comment got cut off a bit. > Source/WebKit/chromium/public/WebFrame.h:285 > + virtual v8::Handle<v8::Value> callFunctionEvenIfScriptDisabled( > + v8::Handle<v8::Function>, > + v8::Handle<v8::Object>, > + int argc, > + v8::Handle<v8::Value> argv[]) = 0; @fishd: Do you have any thoughts on this function? The issue is that for WebKit-related v8::Contexts, we need to route function calls through WebKit for two reasons: (1) to avoid confusing the inspector and (2) to make sure we're correctly running microtasks when the stack unwinds. The complication that arises is that the extension system sometimes needs to call functions even when script is disabled (e.g., to run content scripts in sandboxed iframes). Rafael talked this over for a while, and the best we could come up with was this somewhat-goofy sounding name. Do you think we're doing the right thing here or is there a better way? Thanks!
Created attachment 134475 [details] Patch
Created attachment 134611 [details] Patch
Comment on attachment 134611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134611&action=review > Source/WebKit/chromium/public/WebScriptInvocation.h:38 > +namespace WebScriptInvocation { nit: nested namespaces are uncommon in the webkit API how do you plan to use BypassMicrotaskCheckpoint from chromium?
@fishd: here's the beginning of the chromium side patch: https://chromiumcodereview.appspot.com/9865015 Also, do you have a suggestion about using nested namespace? Would a single class (WebScriptInvocationBypassMicrotaskCheckpoint? BypassMicrotaskCheckpoint?) be preferrable? I'm hoping that we won't ever need to allocate a "naked" WebScriptInvocation (*do* microtask checkpoint), but we may.
(In reply to comment #31) > @fishd: here's the beginning of the chromium side patch: https://chromiumcodereview.appspot.com/9865015 > > Also, do you have a suggestion about using nested namespace? Would a single class (WebScriptInvocationBypassMicrotaskCheckpoint? BypassMicrotaskCheckpoint?) be preferrable? I'm hoping that we won't ever need to allocate a "naked" WebScriptInvocation (*do* microtask checkpoint), but we may. Sorry for being unclear. We normally just define a class within a class in cases like this.
One concern I have is that this seems like quite a complicated aspect of V8 DOM bindings to expose to the embedder. Please document the feature so one doesn't need to study WebCore to understand what it does.
Created attachment 134858 [details] Patch
Created attachment 134876 [details] Patch
API Class now called WebScopedMicrotaskSuppression, per offline discussion with @fishd
Created attachment 134877 [details] Patch
Comment on attachment 134877 [details] Patch I didn't review all of the WebCore changes in detail. That'd surely be better left to someone more familiar with the V8 bindings. View in context: https://bugs.webkit.org/attachment.cgi?id=134877&action=review > Source/WebKit/chromium/public/WebScopedMicrotaskSuppression.h:32 > +#define WebScopedMicrotaskSuppressionn_h oops > Source/WebKit/chromium/public/WebScopedMicrotaskSuppression.h:38 > +// This class wraps V8RecursionScope::BypassMicrotaskCheckpoint. Please Please considering duplicating some of that information here. Two reasons: 1- People who hack on WebCore are not necessarily likely to know to fixup comments in Chromium's WebKit API headers. 2- People who read Chromium's WebKit API headers should not need to look into WebCore to understand the API. > Source/WebKit/chromium/public/WebScopedMicrotaskSuppression.h:42 > + WEBKIT_EXPORT WebScopedMicrotaskSuppression(); There is a convention of not exporting constructors and destructors through the API. Please see WebScopedUserGesture. > Source/WebKit/chromium/src/WebScopedMicrotaskSuppression.cpp:42 > + Impl() { } nit: why bother defining constructor and destructor?
Created attachment 135368 [details] Patch
@fishd: All requested changes made. Out of curiosity: why is it desirable to avoid exposing ctors/dtors to the API? @abarth: Can I get an r+ on the full patch?
Comment on attachment 135368 [details] Patch The WebCore changes look good.
Looks like fishd was happy with the API changes, so I think we're all set.
Comment on attachment 135368 [details] Patch Clearing flags on attachment: 135368 Committed r113111: <http://trac.webkit.org/changeset/113111>
All reviewed patches have been landed. Closing bug.