Created attachment 76587 [details] test case (will assert) There is nothing that guarantees this. See attached test case. I haven't checked what release mode consequences are. In fact, looks like this test also shows another bug: we shouldn't be getting to executeScriptSoon() from error()! #0 0x102e4c866 in WebCore::AsyncScriptRunner::executeScriptSoon at AsyncScriptRunner.cpp:57 #1 0x1038d1ccc in WebCore::ScriptElement::notifyFinished at ScriptElement.cpp:245 #2 0x102ea06aa in WebCore::CachedScript::checkNotify at CachedScript.cpp:100 #3 0x102ea072b in WebCore::CachedScript::error at CachedScript.cpp:108 #4 0x10367f62f in WebCore::Loader::didFail at loader.cpp:213 #5 0x10367f6c7 in WebCore::Loader::didFail at loader.cpp:190 #6 0x103957261 in WebCore::SubresourceLoader::didFail at SubresourceLoader.cpp:200 #7 0x10389cc07 in WebCore::ResourceLoader::didFail at ResourceLoader.cpp:444
This might be a dup of https://bugs.webkit.org/show_bug.cgi?id=49935, I'll look into this.
Created attachment 77564 [details] Patch
> In fact, looks like this test also shows another bug: we shouldn't be getting to executeScriptSoon() from error()! This isn't an issue (other than perhaps misleading naming). The error case is handled in ScriptElement::execute(). void ScriptElement::execute(CachedScript* cachedScript) { ASSERT(cachedScript); if (cachedScript->errorOccurred()) dispatchErrorEvent(); else { evaluateScript(ScriptSourceCode(cachedScript)); dispatchLoadEvent(); } cachedScript->removeClient(this); }
Comment on attachment 77564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77564&action=review Looks good. tHanks. > LayoutTests/fast/dom/HTMLScriptElement/remove-in-beforeload.html:6 > +function obl() Seems this could be a clearer name. :) > LayoutTests/fast/dom/HTMLScriptElement/remove-in-beforeload.html:22 > + s.setAttribute("onbeforeload", "obl()"); > + s.setAttribute("onload", "fail()"); > + s.setAttribute("onerror", "fail()"); > + s.setAttribute("src", "foobar"); I think you can just do s.onbeforeload = obl, but this works too.
Created attachment 77566 [details] Patch for landing
Comment on attachment 77566 [details] Patch for landing + Document* document = m_element->document(); + if (!document || !m_element->inDocument()) How can the document be null?
> Also, it avoids caching the Document references over the arbitrary script execution in the before load event. This is interesting! Can a regression test be added for moving the element to another document in beforeload?
Comment on attachment 77566 [details] Patch for landing Rejecting attachment 77566 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: LMetaElement . fast/dom/HTMLMeterElement ............ fast/dom/HTMLObjectElement .... fast/dom/HTMLObjectElement/form . fast/dom/HTMLOptionElement .... fast/dom/HTMLOutputElement ..... fast/dom/HTMLProgressElement ..... fast/dom/HTMLScriptElement ........... fast/dom/HTMLScriptElement/remove-source.html -> failed Exiting early after 1 failures. 7290 tests run. 147.95s total testing time 7289 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 4 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7327192
I <3 the commit-queue.
Created attachment 77590 [details] Patch
> How can the document be null? Oops. element->document() can never return null, removed the check. > This is interesting! Can a regression test be added for moving the element to another document in beforeload? Good catch. The test case exposed a bug with my logic. Just checking element->inDocument() doesn't work if the element was added to a new document. So this new patch introduces this really hacky m_removals variable. If the removedFromDocument() method was ever called during the beforeload event, then requestScript should bail out. I'd rather not commit this approach with an m_removals counter. I uploaded the patch because I can't think of a better pattern, and am wondering if you know of a better pattern.
(In reply to comment #11) > > This is interesting! Can a regression test be added for moving the element to another document in beforeload? > > Good catch. The test case exposed a bug with my logic. Just checking element->inDocument() doesn't work if the element was added to a new document. > > So this new patch introduces this really hacky m_removals variable. If the removedFromDocument() method was ever called during the beforeload event, then requestScript should bail out. I'd rather not commit this approach with an m_removals counter. I uploaded the patch because I can't think of a better pattern, and am wondering if you know of a better pattern. Wait, but what if we removed the script element and inserted back into the same document on beforeunload? Shouldn't we execute the script in that case?
Comment on attachment 77590 [details] Patch Why not cache the document pointer and check that against the current document pointer? If the documetn is not the same, abort.
I don't know if you'd have to store the cached Document pointer in a RefPtr or not.
> Wait, but what if we removed the script element and inserted back into the same document on beforeunload? Shouldn't we execute the script in that case? Yes, that is how it works. insertedIntoDocument() will invoke another requestScript() while the former requestScript() is still on the stack. That's why the original one needs to bail out.
(In reply to comment #13) > (From update of attachment 77590 [details]) > Why not cache the document pointer and check that against the current document pointer? If the documetn is not the same, abort. Isn't there a non-zero chance that the old doc could be destroyed and a new doc could be allocated at the same location? If not, that would be pretty clean.
> Isn't there a non-zero chance that the old doc could be destroyed and a new doc could be allocated at the same location? If not, that would be pretty clean. Oh wait, a RefPtr would prevent that (like you suggested).
Created attachment 77591 [details] Patch
Thinking about this some more, this doesn't seem to be specific to script elements. I wonder if this check should go in ContainerNode:dispatchBeforeLoadEvent(). That would also prevent the extra overhead in the common case where the document doesn't have beforeload listeners. If no one can think of any elements this doesn't apply to, I'll try moving it up.
(In reply to comment #19) > Thinking about this some more, this doesn't seem to be specific to script elements. I wonder if this check should go in ContainerNode:dispatchBeforeLoadEvent(). That would also prevent the extra overhead in the common case where the document doesn't have beforeload listeners. If no one can think of any elements this doesn't apply to, I'll try moving it up. Nevermind. This is element specific. See: http://trac.webkit.org/changeset/49313 This patch sticks to only doing it for script elements. Antti is taking care of it for link elements in https://bugs.webkit.org/show_bug.cgi?id=8852 (That patch should probably check for a move in addition to a remove). PTAL
Comment on attachment 77591 [details] Patch r=me
(In reply to comment #20) > This patch sticks to only doing it for script elements. Antti is taking care of it for link elements in https://bugs.webkit.org/show_bug.cgi?id=8852 (That patch should probably check for a move in addition to a remove). Feel free to fix the stylesheet issue separately as well. Test fast/dom/beforeload/remove-link-in-beforeload-listener.html should cover the removal case but doesn't work correctly atm and that patch in bug 8852 exposed this. That test could be tweaked to work independently and the move case would really need a test of its own.
The commit-queue encountered the following flaky tests while processing attachment 77591 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: dumi@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 77591 [details] Patch Clearing flags on attachment: 77591 Committed r74752: <http://trac.webkit.org/changeset/74752>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/74752 might have broken Leopard Intel Debug (Tests)