Bug 75533 – Move the check for canExecuteScripts out of V8Proxy::retrieve

• Component: New Bugs
• Status: RESOLVED FIXED

Move the check for canExecuteScripts out of V8Proxy::retrieve

Created attachment 121090 [details]
Patch

[WebKit Review Bot]

Comment on attachment 121090 [details]
Patch

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

New failing tests:
editing/pasteboard/drag-image-in-about-blank-frame.html

[Adam Barth]

Comment on attachment 121090 [details]
Patch

This is great.  Are these all the callsitse to retrieve?

(In reply to comment #3)
> (From update of attachment 121090 [details])
> This is great.  Are these all the callsitse to retrieve?

Yes, that's about it.

I'm going to add tests next

[Adam Barth]

Comment on attachment 121090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121090&action=review

> Source/WebCore/bindings/v8/V8Proxy.cpp:-510
> -    return frame->script()->canExecuteScripts(NotAboutToExecuteScript) ? frame->script()->proxy() : 0;

So, these all used to be NotAboutToExecuteScript, but the checks you've added us AboutToExecuteScript.  We probably want to change them to keep them as NotAboutToExecuteScript.

Created attachment 121561 [details]
Patch

(In reply to comment #5)
> (From update of attachment 121090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121090&action=review
> 
> > Source/WebCore/bindings/v8/V8Proxy.cpp:-510
> > -    return frame->script()->canExecuteScripts(NotAboutToExecuteScript) ? frame->script()->proxy() : 0;
> 
> So, these all used to be NotAboutToExecuteScript, but the checks you've added us AboutToExecuteScript.  We probably want to change them to keep them as NotAboutToExecuteScript.

PTAL

In my original CL, I tried to not put the checks at sites where I think they shouldn't go. I think that's too risky for a single CL, so I updated the patch to do the check everywhere, it's now a simple refactoring.

I'll inspect all call-sites for where the check should not be made in follow-up CLs

[Adam Barth]

Comment on attachment 121561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121561&action=review

> Source/WebCore/bindings/v8/ScheduledAction.cpp:100
> +        if (!scriptController->canExecuteScripts(NotAboutToExecuteScript))

Shouldn't this be AboutToExecuteScript?  We're calling execute two lines down...

> Source/WebCore/bindings/v8/V8EventListener.cpp:88
> +        if (frame->script()->canExecuteScripts(NotAboutToExecuteScript))

Isn't this AboutToExecuteScript?  We're calling a function on the next line.

> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:70
> +        if (frame->script()->canExecuteScripts(NotAboutToExecuteScript))

ditto

Created attachment 121995 [details]
Patch

for the record, I'll change NotAboutToExecuteScript to AboutExecuteScript where appropriate in a follow-up change. This change is supposed to just move the existing check which is NotAboutToExecuteScript.

I've also dropped the changes to the custom bindings, because they won't get executed when scripts are disabled.

[WebKit Review Bot]

Comment on attachment 121995 [details]
Patch

Clearing flags on attachment: 121995

Committed r104694: <http://trac.webkit.org/changeset/104694>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

I think this patch made http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/security/sandbox-inherit-to-initial-document-2.html start passing, which concerns me a bit since it's a security test and this patch was described as "No functionality change":

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fsecurity%2Fsandbox-inherit-to-initial-document-2.html


Can someone verify that this is expected and see if there are any other implications?

[Adam Barth]

> Can someone verify that this is expected and see if there are any other implications?

Interesting.  That's a result of this decision:

> I've also dropped the changes to the custom bindings, because they won't get executed when scripts are disabled.

Apparently, these cases are visible in the case where script is disabled via the sandbox attribute by same-origin access is allowed.  In that case, the outer frame can get a wrapper for the DOMWindow of the inner frame, which previously was impossible.

If we had foreseen that effect, we probably would have made that change separately.  However, that change is indeed a progression.  I think it's ok to leave as-is.

Thanks for spotting this James.