Bug 111720

Summary: Wrong parameters passed to canExecuteScripts in ScriptEventListener (JSC and V8)
Product: WebKit Reporter: Xan Lopez <xan.lopez>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: NEW    
Severity: Normal CC: abarth, buildbot, dglazkov, fpizlo, ggaren, haraken, japhet, joenotcharles, oliver, rniwa, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 111729    
Attachments:
Description Flags
canexecutescripts.diff haraken: review-, webkit.review.bot: commit-queue-

Xan Lopez
Reported 2013-03-07 06:09:08 PST
Not 100% sure about this, but I think that the patch in bug #35205 (old one!) made the wrong change in a couple places for ScriptEventListener, for both JSC and V8. In particular the calls that happen in the methods used to create the event listeners say that they are about to execute JS code, but AFAICT they are not (they just create the event listener object). If this is correct I can quickly make the patch, although I'm not sure of how I could test this.
Attachments
canexecutescripts.diff (4.21 KB, patch)
2013-03-07 07:17 PST, Xan Lopez 		haraken: review-
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Xan Lopez
Comment 1 2013-03-07 07:17:54 PST
Created attachment 191991 [details] canexecutescripts.diff For reference, this would the patch.
Kentaro Hara
Comment 2 2013-03-07 07:29:43 PST
Comment on attachment 191991 [details] canexecutescripts.diff How does this patch affect actual behavior?
Joe Mason
Comment 3 2013-03-07 10:18:33 PST
I don't even remember writing the patch that this is based on anymore, but looking at the code... The only difference that the AboutToExecuteScript parameter makes is that if it's set, and canExecuteScripts returns false, FrameLoaderClient::didNotAllowScript gets called. Which could, for example, open a dialog warning the user that scripts on this page did not run. But createAttributeEventListener does not actually run a script - it just creates a listener. The script isn't run unless the listener fires. So without this patch, if you disable Javascript you could get a "Javascript was blocked" warning just on loading the page, rather than when performing the action that actually runs the script. Looking at the code, I think JSLazyEventListener::initializeJSFunction has the same problem. In JSEventListener::handleEvent, canExecuteScripts(AboutToExecuteScript) gets called before the script is executed*, so the order is: JSEventListener::handleEvent calls JSEventListener::jsFunction calls JSLazyEventListener::initializeJSFunction calls canExecuteScripts(AboutToExecuteScript) -> should be NotAboutToExecuteScript, since it's just creating and returning the script, not executing it yet calls canExecuteScripts(AboutToExecuteScript) executes the script If initializeJSFunction is ever called in a different context where the script is just held for a while before executing, this would cause didNotAllowScripts to be called prematurely. *(But only for documents, with "FIXME: Is this check needed for other contexts? Should look into that. It's possible that JSLazyEventListener always calling canExecuteScripts(AboutToExecuteScript) is hiding another bug where it's not always called in JSEventListener::handleEvent.)
WebKit Review Bot
Comment 4 2013-03-07 12:45:10 PST
Comment on attachment 191991 [details] canexecutescripts.diff Attachment 191991 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17027165 New failing tests: media/video-controls-no-scripting.html fast/frames/sandboxed-iframe-autofocus-denied.html
Build Bot
Comment 5 2013-03-07 17:00:52 PST
Comment on attachment 191991 [details] canexecutescripts.diff Attachment 191991 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16991401 New failing tests: media/video-controls-no-scripting.html fast/frames/sandboxed-iframe-autofocus-denied.html
Kentaro Hara
Comment 6 2013-03-07 17:06:30 PST
Comment on attachment 191991 [details] canexecutescripts.diff This change may be correct but may not be correct. It looks like more investigation is needed. Let me r- it for now.
Xan Lopez
Comment 7 2013-05-28 03:02:16 PDT
At the very least we can remove the V8 stuff now, but I wonder what the JSC folks think about the patch. CCing a few people.
Filip Pizlo
Comment 8 2013-05-28 07:24:24 PDT
What bug does this fix?
Note You need to log in before you can comment on or make changes to this bug.