Bug 79131 - Use V8 completion callback API to assert that V8RecursionScope is on the stack whenever invoking script
Summary: Use V8 completion callback API to assert that V8RecursionScope is on the stac...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2012-02-21 11:07 PST by Adam Klein
Modified: 2012-04-03 17:28 PDT (History)
14 users (show)

See Also:


Attachments
Patch (34.41 KB, patch)
2012-02-21 11:19 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Fix release build (34.46 KB, patch)
2012-02-24 11:55 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (42.89 KB, patch)
2012-03-26 18:35 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (46.29 KB, patch)
2012-03-27 16:56 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (37.73 KB, patch)
2012-03-28 18:38 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (37.73 KB, patch)
2012-03-28 18:41 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (45.71 KB, patch)
2012-03-28 19:01 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (45.72 KB, patch)
2012-03-29 10:16 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (45.95 KB, patch)
2012-03-30 12:16 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (46.50 KB, patch)
2012-03-30 14:03 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (46.50 KB, patch)
2012-03-30 14:05 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (46.57 KB, patch)
2012-04-03 11:09 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2012-02-21 11:07:48 PST
Use V8 completion callback API to assert that V8RecursionScope is on the stack whenever invoking script
Comment 1 Adam Klein 2012-02-21 11:19:04 PST
Created attachment 128004 [details]
Patch
Comment 2 Adam Klein 2012-02-21 11:46:50 PST
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 3 Adam Barth 2012-02-21 15:12:21 PST
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.
Comment 4 Adam Barth 2012-02-21 15:13:20 PST
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 5 WebKit Review Bot 2012-02-21 15:34:41 PST
Comment on attachment 128004 [details]
Patch

Attachment 128004 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11560280
Comment 6 Adam Klein 2012-02-21 16:05:55 PST
(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.
Comment 7 Adam Barth 2012-02-21 16:14:56 PST
Hum...  Maybe if you try to create a page author scope inside of a non-page author scope, that should fire an ASSERT?
Comment 8 Adam Barth 2012-02-21 16:16:16 PST
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.
Comment 9 Adam Klein 2012-02-21 16:18:41 PST
(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.
Comment 10 Adam Klein 2012-02-24 11:55:11 PST
Created attachment 128778 [details]
Fix release build
Comment 11 Adam Klein 2012-02-24 11:57:08 PST
Pavel, I'd be interested on your thoughts on the changes in ScriptDebugServer.cpp. Does this make any sense to you?
Comment 12 Rafael Weinstein 2012-03-26 18:35:27 PDT
Created attachment 133957 [details]
Patch
Comment 13 Rafael Weinstein 2012-03-26 18:53:22 PDT
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?
Comment 14 Rafael Weinstein 2012-03-26 18:57:40 PDT
Also, here's the beginnings of a patch of what this would look like from the chromium renderer side:

https://chromiumcodereview.appspot.com/9865015
Comment 15 Adam Barth 2012-03-26 22:45:58 PDT
> 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.
Comment 16 Rafael Weinstein 2012-03-27 05:09:45 PDT
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 17 Adam Barth 2012-03-27 13:59:41 PDT
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.
Comment 18 Adam Barth 2012-03-27 14:03:25 PDT
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?
Comment 19 Adam Barth 2012-03-27 14:04:22 PDT
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?
Comment 20 Rafael Weinstein 2012-03-27 16:56:14 PDT
Created attachment 134169 [details]
Patch
Comment 21 WebKit Review Bot 2012-03-27 17:00:19 PDT
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.
Comment 22 Adam Barth 2012-03-27 17:01:27 PDT
> 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.
Comment 23 Rafael Weinstein 2012-03-27 17:05:05 PDT
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 24 Adam Barth 2012-03-27 23:47:42 PDT
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 ?
Comment 25 Rafael Weinstein 2012-03-28 18:38:42 PDT
Created attachment 134466 [details]
Patch
Comment 26 Rafael Weinstein 2012-03-28 18:41:30 PDT
Created attachment 134467 [details]
Patch
Comment 27 Adam Barth 2012-03-28 18:55:11 PDT
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!
Comment 28 Rafael Weinstein 2012-03-28 19:01:51 PDT
Created attachment 134475 [details]
Patch
Comment 29 Rafael Weinstein 2012-03-29 10:16:18 PDT
Created attachment 134611 [details]
Patch
Comment 30 Darin Fisher (:fishd, Google) 2012-03-29 12:00:15 PDT
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?
Comment 31 Rafael Weinstein 2012-03-29 12:09:12 PDT
@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.
Comment 32 Darin Fisher (:fishd, Google) 2012-03-29 21:42:39 PDT
(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.
Comment 33 Darin Fisher (:fishd, Google) 2012-03-29 21:43:32 PDT
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.
Comment 34 Rafael Weinstein 2012-03-30 12:16:28 PDT
Created attachment 134858 [details]
Patch
Comment 35 Rafael Weinstein 2012-03-30 14:03:15 PDT
Created attachment 134876 [details]
Patch
Comment 36 Rafael Weinstein 2012-03-30 14:04:19 PDT
API Class now called WebScopedMicrotaskSuppression, per offline discussion with @fishd
Comment 37 Rafael Weinstein 2012-03-30 14:05:37 PDT
Created attachment 134877 [details]
Patch
Comment 38 Darin Fisher (:fishd, Google) 2012-04-02 15:02:04 PDT
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?
Comment 39 Rafael Weinstein 2012-04-03 11:09:57 PDT
Created attachment 135368 [details]
Patch
Comment 40 Rafael Weinstein 2012-04-03 11:14:07 PDT
@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 41 Adam Barth 2012-04-03 15:29:36 PDT
Comment on attachment 135368 [details]
Patch

The WebCore changes look good.
Comment 42 Adam Barth 2012-04-03 15:31:24 PDT
Looks like fishd was happy with the API changes, so I think we're all set.
Comment 43 WebKit Review Bot 2012-04-03 17:28:42 PDT
Comment on attachment 135368 [details]
Patch

Clearing flags on attachment: 135368

Committed r113111: <http://trac.webkit.org/changeset/113111>
Comment 44 WebKit Review Bot 2012-04-03 17:28:50 PDT
All reviewed patches have been landed.  Closing bug.