Bug 75533

Summary: Move the check for canExecuteScripts out of V8Proxy::retrieve
Product: WebKit Reporter: jochen
Component: New BugsAssignee: jochen
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, jamesr, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description jochen 2012-01-04 03:09:49 PST
Move the check for canExecuteScripts out of V8Proxy::retrieve
Comment 1 jochen 2012-01-04 03:10:32 PST
Created attachment 121090 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-04 05:47:29 PST
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
Comment 3 Adam Barth 2012-01-04 07:45:15 PST
Comment on attachment 121090 [details]
Patch

This is great.  Are these all the callsitse to retrieve?
Comment 4 jochen 2012-01-05 10:45:34 PST
(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
Comment 5 Adam Barth 2012-01-05 11:38:07 PST
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.
Comment 6 jochen 2012-01-07 16:11:43 PST
Created attachment 121561 [details]
Patch
Comment 7 jochen 2012-01-07 16:15:16 PST
(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
Comment 8 Adam Barth 2012-01-10 12:47:07 PST
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
Comment 9 jochen 2012-01-11 02:26:00 PST
Created attachment 121995 [details]
Patch
Comment 10 jochen 2012-01-11 02:41:33 PST
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.
Comment 11 WebKit Review Bot 2012-01-11 02:58:15 PST
Comment on attachment 121995 [details]
Patch

Clearing flags on attachment: 121995

Committed r104694: <http://trac.webkit.org/changeset/104694>
Comment 12 WebKit Review Bot 2012-01-11 02:58:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 James Robinson 2012-01-13 11:44:15 PST
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?
Comment 14 Adam Barth 2012-01-13 12:21:26 PST
> 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.