Bug 111720 - Wrong parameters passed to canExecuteScripts in ScriptEventListener (JSC and V8)
Summary: Wrong parameters passed to canExecuteScripts in ScriptEventListener (JSC and V8)
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 111729
  Show dependency treegraph
 
Reported: 2013-03-07 06:09 PST by Xan Lopez
Modified: 2013-05-28 07:24 PDT (History)
12 users (show)

See Also:


Attachments
canexecutescripts.diff (4.21 KB, patch)
2013-03-07 07:17 PST, Xan Lopez
haraken: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 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.
Comment 1 Xan Lopez 2013-03-07 07:17:54 PST
Created attachment 191991 [details]
canexecutescripts.diff

For reference, this would the patch.
Comment 2 Kentaro Hara 2013-03-07 07:29:43 PST
Comment on attachment 191991 [details]
canexecutescripts.diff

How does this patch affect actual behavior?
Comment 3 Joe Mason 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.)
Comment 4 WebKit Review Bot 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
Comment 5 Build Bot 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
Comment 6 Kentaro Hara 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.
Comment 7 Xan Lopez 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.
Comment 8 Filip Pizlo 2013-05-28 07:24:24 PDT
What bug does this fix?