Bug 79131 – Use V8 completion callback API to assert that V8RecursionScope is on the stack whenever invoking script

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Use V8 completion callback API to assert that V8RecursionScope is on the stack whenever invoking script

[Adam Klein]

Created attachment 128004 [details]
Patch

[Adam Klein]

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).

[Adam Barth]

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.

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 128004 [details]
Patch

Attachment 128004 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11560280

[Adam Klein]

(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.

[Adam Barth]

Hum...  Maybe if you try to create a page author scope inside of a non-page author scope, that should fire an ASSERT?

[Adam Barth]

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.

[Adam Klein]

(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.

[Adam Klein]

Created attachment 128778 [details]
Fix release build

[Adam Klein]

Pavel, I'd be interested on your thoughts on the changes in ScriptDebugServer.cpp. Does this make any sense to you?

[Rafael Weinstein]

Created attachment 133957 [details]
Patch

[Rafael Weinstein]

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?

[Rafael Weinstein]

Also, here's the beginnings of a patch of what this would look like from the chromium renderer side:

https://chromiumcodereview.appspot.com/9865015

[Adam Barth]

> 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.

[Rafael Weinstein]

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.."

[Adam Barth]

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.

[Adam Barth]

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?

[Adam Barth]

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?

[Rafael Weinstein]

Created attachment 134169 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

> 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.

[Rafael Weinstein]

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.

[Adam Barth]

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 ?

[Rafael Weinstein]

Created attachment 134466 [details]
Patch

[Rafael Weinstein]

Created attachment 134467 [details]
Patch

[Adam Barth]

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!

[Rafael Weinstein]

Created attachment 134475 [details]
Patch

[Rafael Weinstein]

Created attachment 134611 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[Rafael Weinstein]

@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.

[Darin Fisher (:fishd, Google)]

(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.

[Darin Fisher (:fishd, Google)]

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.

[Rafael Weinstein]

Created attachment 134858 [details]
Patch

[Rafael Weinstein]

Created attachment 134876 [details]
Patch

[Rafael Weinstein]

API Class now called WebScopedMicrotaskSuppression, per offline discussion with @fishd

[Rafael Weinstein]

Created attachment 134877 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[Rafael Weinstein]

Created attachment 135368 [details]
Patch

[Rafael Weinstein]

@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?

[Adam Barth]

Comment on attachment 135368 [details]
Patch

The WebCore changes look good.

[Adam Barth]

Looks like fishd was happy with the API changes, so I think we're all set.

[WebKit Review Bot]

Comment on attachment 135368 [details]
Patch

Clearing flags on attachment: 135368

Committed r113111: <http://trac.webkit.org/changeset/113111>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.