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.
Created attachment 191991 [details] canexecutescripts.diff For reference, this would the patch.
Comment on attachment 191991 [details] canexecutescripts.diff How does this patch affect actual behavior?
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 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 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 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.
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.
What bug does this fix?