Dispatch events in isolated worlds, even if script execution is forbidden

Dispatch events in isolated worlds, even if script execution is forbidden

BUG=608286, 607072
R=mkwst@chromium.org

Committed: https://crrev.com/866d1237c72059624def2242e218a7dfe78b125e
Cr-Commit-Position: refs/heads/master@{#391197}

[haraken, 2016-05-02 09:23:55]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-05-02 09:23:55]

https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right):

https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
(scriptState->world().isMainWorld() &&
!frame->script().canExecuteScripts(AboutToExecuteScript))

Would it be better to move the check into canExecuteScripts? (assuming that you
really want to enable all extension stuff when JS is disabled.)

[Mike West, 2016-05-02 09:30:15]

https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right):

https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
(scriptState->world().isMainWorld() &&
!frame->script().canExecuteScripts(AboutToExecuteScript))
On 2016/05/02 at 09:23:55, haraken wrote:
> Would it be better to move the check into canExecuteScripts? (assuming that
you really want to enable all extension stuff when JS is disabled.)

I don't understand the patch, actually: why doesn't the call to
`isInPrivateScriptIsolateWorld` in `canExecuteScripts` already handle this case?

[haraken, 2016-05-02 09:31:45]

On 2016/05/02 09:30:15, Mike West wrote:
>
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right):
> 
>
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
> (scriptState->world().isMainWorld() &&
> !frame->script().canExecuteScripts(AboutToExecuteScript))
> On 2016/05/02 at 09:23:55, haraken wrote:
> > Would it be better to move the check into canExecuteScripts? (assuming that
> you really want to enable all extension stuff when JS is disabled.)
> 
> I don't understand the patch, actually: why doesn't the call to
> `isInPrivateScriptIsolateWorld` in `canExecuteScripts` already handle this
case?

Because a private script (i.e., Blink-in-JS) is a part of Blink's
implementation, so it should always be enabled regardless of whether JS is
enabled or disabled.

[Mike West, 2016-05-02 09:40:12]

On 2016/05/02 at 09:31:45, haraken wrote:
> On 2016/05/02 09:30:15, Mike West wrote:
> >
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right):
> > 
> >
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
> > (scriptState->world().isMainWorld() &&
> > !frame->script().canExecuteScripts(AboutToExecuteScript))
> > On 2016/05/02 at 09:23:55, haraken wrote:
> > > Would it be better to move the check into canExecuteScripts? (assuming
that
> > you really want to enable all extension stuff when JS is disabled.)
> > 
> > I don't understand the patch, actually: why doesn't the call to
> > `isInPrivateScriptIsolateWorld` in `canExecuteScripts` already handle this
case?
> 
> Because a private script (i.e., Blink-in-JS) is a part of Blink's
implementation, so it should always be enabled regardless of whether JS is
enabled or disabled.

Interesting. I thought we removed that entirely.

I think I agree that `canExecuteScripts` ought to be handling this check; it
seems like you'd need to add it in more places to allow extensions to do all the
things that extensions like to do, so centralizing the check seems advisable.

[jochen (gone - plz use gerrit), 2016-05-02 09:41:52]

https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right):

https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
(scriptState->world().isMainWorld() &&
!frame->script().canExecuteScripts(AboutToExecuteScript))
On 2016/05/02 at 09:23:55, haraken wrote:
> Would it be better to move the check into canExecuteScripts? (assuming that
you really want to enable all extension stuff when JS is disabled.)

then if an isolated world inserted a script into the main world, because we're
still in the context of the isolated world when inserting the element.

https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
(scriptState->world().isMainWorld() &&
!frame->script().canExecuteScripts(AboutToExecuteScript))
On 2016/05/02 at 09:23:55, haraken wrote:
> Would it be better to move the check into canExecuteScripts? (assuming that
you really want to enable all extension stuff when JS is disabled.)

then if an isolated world inserted a script into the main world, because we're
still in the context of the isolated world when inserting the element.

[haraken, 2016-05-02 09:45:59]

https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right):

https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
(scriptState->world().isMainWorld() &&
!frame->script().canExecuteScripts(AboutToExecuteScript))
On 2016/05/02 09:41:52, jochen wrote:
> On 2016/05/02 at 09:23:55, haraken wrote:
> > Would it be better to move the check into canExecuteScripts? (assuming that
> you really want to enable all extension stuff when JS is disabled.)
> 
> then if an isolated world inserted a script into the main world, because we're
> still in the context of the isolated world when inserting the element.

Your point is that canExecuteScripts should take a ScriptState parameter and use
the ScriptState instead of relying on isolate->GetCurrentContext()? (That makes
sense to me.)

[jochen (gone - plz use gerrit), 2016-05-02 09:50:10]

On 2016/05/02 at 09:45:59, haraken wrote:
>
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right):
> 
>
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
(scriptState->world().isMainWorld() &&
!frame->script().canExecuteScripts(AboutToExecuteScript))
> On 2016/05/02 09:41:52, jochen wrote:
> > On 2016/05/02 at 09:23:55, haraken wrote:
> > > Would it be better to move the check into canExecuteScripts? (assuming
that
> > you really want to enable all extension stuff when JS is disabled.)
> > 
> > then if an isolated world inserted a script into the main world, because
we're
> > still in the context of the isolated world when inserting the element.
> 
> Your point is that canExecuteScripts should take a ScriptState parameter and
use the ScriptState instead of relying on isolate->GetCurrentContext()? (That
makes sense to me.)

except, we don't have a script state everywhere we call that method :(

also, I'd argue that the check for private scripts is wrong and only works
because none of the scripts that depend on it are doing stuff like inserting
script tags

[haraken, 2016-05-02 10:08:34]

On 2016/05/02 09:50:10, jochen wrote:
> On 2016/05/02 at 09:45:59, haraken wrote:
> >
>
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
> (scriptState->world().isMainWorld() &&
> !frame->script().canExecuteScripts(AboutToExecuteScript))
> > On 2016/05/02 09:41:52, jochen wrote:
> > > On 2016/05/02 at 09:23:55, haraken wrote:
> > > > Would it be better to move the check into canExecuteScripts? (assuming
> that
> > > you really want to enable all extension stuff when JS is disabled.)
> > > 
> > > then if an isolated world inserted a script into the main world, because
> we're
> > > still in the context of the isolated world when inserting the element.
> > 
> > Your point is that canExecuteScripts should take a ScriptState parameter and
> use the ScriptState instead of relying on isolate->GetCurrentContext()? (That
> makes sense to me.)
> 
> except, we don't have a script state everywhere we call that method :(
> 
> also, I'd argue that the check for private scripts is wrong and only works
> because none of the scripts that depend on it are doing stuff like inserting
> script tags

Agreed. It would be wrong for the private scripts to use
isolate->GetCurrentContext -- it should pass in a correct ScriptState to
canExecuteScripts.

> except, we don't have a script state everywhere we call that method :(

Maybe should we pass in a DOMWrapperWorld? A ScriptState may not always be
available, but a DOMWrapperWorld should be clarified when we're about to execute
a script.

[jochen (gone - plz use gerrit), 2016-05-02 10:27:54]

On 2016/05/02 at 10:08:34, haraken wrote:
> On 2016/05/02 09:50:10, jochen wrote:
> > On 2016/05/02 at 09:45:59, haraken wrote:
> > >
> >
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp
(right):
> > > 
> > >
> >
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
> > (scriptState->world().isMainWorld() &&
> > !frame->script().canExecuteScripts(AboutToExecuteScript))
> > > On 2016/05/02 09:41:52, jochen wrote:
> > > > On 2016/05/02 at 09:23:55, haraken wrote:
> > > > > Would it be better to move the check into canExecuteScripts? (assuming
> > that
> > > > you really want to enable all extension stuff when JS is disabled.)
> > > > 
> > > > then if an isolated world inserted a script into the main world, because
> > we're
> > > > still in the context of the isolated world when inserting the element.
> > > 
> > > Your point is that canExecuteScripts should take a ScriptState parameter
and
> > use the ScriptState instead of relying on isolate->GetCurrentContext()?
(That
> > makes sense to me.)
> > 
> > except, we don't have a script state everywhere we call that method :(
> > 
> > also, I'd argue that the check for private scripts is wrong and only works
> > because none of the scripts that depend on it are doing stuff like inserting
> > script tags
> 
> Agreed. It would be wrong for the private scripts to use
isolate->GetCurrentContext -- it should pass in a correct ScriptState to
canExecuteScripts.
> 
> > except, we don't have a script state everywhere we call that method :(
> 
> Maybe should we pass in a DOMWrapperWorld? A ScriptState may not always be
available, but a DOMWrapperWorld should be clarified when we're about to execute
a script.

in most cases this would mean statically passing in the main world - the API is
often used to just check the flag without executing any script.

[haraken, 2016-05-02 10:47:37]

On 2016/05/02 10:27:54, jochen wrote:
> On 2016/05/02 at 10:08:34, haraken wrote:
> > On 2016/05/02 09:50:10, jochen wrote:
> > > On 2016/05/02 at 09:45:59, haraken wrote:
> > > >
> > >
>
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> > > > File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp
> (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1939713003/diff/1/third_party/WebKit/Source/b...
> > > > third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:89: if
> > > (scriptState->world().isMainWorld() &&
> > > !frame->script().canExecuteScripts(AboutToExecuteScript))
> > > > On 2016/05/02 09:41:52, jochen wrote:
> > > > > On 2016/05/02 at 09:23:55, haraken wrote:
> > > > > > Would it be better to move the check into canExecuteScripts?
(assuming
> > > that
> > > > > you really want to enable all extension stuff when JS is disabled.)
> > > > > 
> > > > > then if an isolated world inserted a script into the main world,
because
> > > we're
> > > > > still in the context of the isolated world when inserting the element.
> > > > 
> > > > Your point is that canExecuteScripts should take a ScriptState parameter
> and
> > > use the ScriptState instead of relying on isolate->GetCurrentContext()?
> (That
> > > makes sense to me.)
> > > 
> > > except, we don't have a script state everywhere we call that method :(
> > > 
> > > also, I'd argue that the check for private scripts is wrong and only works
> > > because none of the scripts that depend on it are doing stuff like
inserting
> > > script tags
> > 
> > Agreed. It would be wrong for the private scripts to use
> isolate->GetCurrentContext -- it should pass in a correct ScriptState to
> canExecuteScripts.
> > 
> > > except, we don't have a script state everywhere we call that method :(
> > 
> > Maybe should we pass in a DOMWrapperWorld? A ScriptState may not always be
> available, but a DOMWrapperWorld should be clarified when we're about to
execute
> a script.
> 
> in most cases this would mean statically passing in the main world - the API
is
> often used to just check the flag without executing any script.

Then can we make the world parameter a default parameter pointing to the main
world?

[jochen (gone - plz use gerrit), 2016-05-02 16:05:51]

What about me filling a bug to consider doing this? I think changing this all
over the place requires a careful review of all callsites as I don't want to
accidentally allow running scripts if something gets hold of the wrong script
state.

[haraken, 2016-05-02 17:04:32]

On 2016/05/02 16:05:51, jochen wrote:
> What about me filling a bug to consider doing this? I think changing this all
> over the place requires a careful review of all callsites as I don't want to
> accidentally allow running scripts if something gets hold of the wrong script
> state.

LGTM, but add a TODO on the code you're introducing here.

[jochen (gone - plz use gerrit), 2016-05-03 08:51:11]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-05-03 08:51:12]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1939713003/#ps20001
(title: "updates")

[commit-bot: I haz the power, 2016-05-03 08:51:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939713003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939713003/20001

[commit-bot: I haz the power, 2016-05-03 11:08:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-03 11:10:12]

Description was changed from

==========
Dispatch events in isolated worlds, even if script execution is forbidden

BUG=608286,607072
R=mkwst@chromium.org
==========

to

==========
Dispatch events in isolated worlds, even if script execution is forbidden

BUG=608286,607072
R=mkwst@chromium.org

Committed: https://crrev.com/866d1237c72059624def2242e218a7dfe78b125e
Cr-Commit-Position: refs/heads/master@{#391197}
==========

[commit-bot: I haz the power, 2016-05-03 11:10:13]

Patchset 2 (id:??) landed as
https://crrev.com/866d1237c72059624def2242e218a7dfe78b125e
Cr-Commit-Position: refs/heads/master@{#391197}