Summary: | Move the check for canExecuteScripts out of V8Proxy::retrieve | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||||
Component: | New Bugs | Assignee: | 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
jochen
2012-01-04 03:09:49 PST
Created attachment 121090 [details]
Patch
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 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 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 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. Comment on attachment 121995 [details] Patch Clearing flags on attachment: 121995 Committed r104694: <http://trac.webkit.org/changeset/104694> All reviewed patches have been landed. Closing bug. 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? > 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. |