RESOLVED FIXED 51067
Assertion failure: element->inDocument() in AsyncScriptRunner::executeScriptSoon()
https://bugs.webkit.org/show_bug.cgi?id=51067
Summary Assertion failure: element->inDocument() in AsyncScriptRunner::executeScriptS...
Alexey Proskuryakov
Reported 2010-12-14 15:49:35 PST
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
Attachments
test case (will assert) (592 bytes, text/html)
2010-12-14 15:49 PST, Alexey Proskuryakov
no flags
Patch (4.28 KB, patch)
2010-12-28 11:22 PST, Tony Gentilcore
no flags
Patch for landing (4.24 KB, patch)
2010-12-28 11:38 PST, Tony Gentilcore
no flags
Patch (8.39 KB, patch)
2010-12-28 16:32 PST, Tony Gentilcore
no flags
Patch (7.55 KB, patch)
2010-12-28 16:58 PST, Tony Gentilcore
no flags
Tony Gentilcore
Comment 1 2010-12-14 16:03:29 PST
This might be a dup of https://bugs.webkit.org/show_bug.cgi?id=49935, I'll look into this.
Tony Gentilcore
Comment 2 2010-12-28 11:22:36 PST
Tony Gentilcore
Comment 3 2010-12-28 11:24:38 PST
> 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); }
Eric Seidel (no email)
Comment 4 2010-12-28 11:29:08 PST
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.
Tony Gentilcore
Comment 5 2010-12-28 11:38:37 PST
Created attachment 77566 [details] Patch for landing
Alexey Proskuryakov
Comment 6 2010-12-28 11:54:25 PST
Comment on attachment 77566 [details] Patch for landing + Document* document = m_element->document(); + if (!document || !m_element->inDocument()) How can the document be null?
Alexey Proskuryakov
Comment 7 2010-12-28 11:57:04 PST
> 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?
WebKit Commit Bot
Comment 8 2010-12-28 13:34:32 PST
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
Eric Seidel (no email)
Comment 9 2010-12-28 13:36:04 PST
I <3 the commit-queue.
Tony Gentilcore
Comment 10 2010-12-28 16:32:33 PST
Tony Gentilcore
Comment 11 2010-12-28 16:37:16 PST
> 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.
Ryosuke Niwa
Comment 12 2010-12-28 16:41:27 PST
(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?
Eric Seidel (no email)
Comment 13 2010-12-28 16:42:45 PST
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.
Eric Seidel (no email)
Comment 14 2010-12-28 16:43:10 PST
I don't know if you'd have to store the cached Document pointer in a RefPtr or not.
Tony Gentilcore
Comment 15 2010-12-28 16:44:06 PST
> 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.
Tony Gentilcore
Comment 16 2010-12-28 16:45:38 PST
(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.
Tony Gentilcore
Comment 17 2010-12-28 16:47:26 PST
> 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).
Tony Gentilcore
Comment 18 2010-12-28 16:58:36 PST
Tony Gentilcore
Comment 19 2010-12-28 17:07:08 PST
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.
Tony Gentilcore
Comment 20 2010-12-29 12:00:19 PST
(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
Antti Koivisto
Comment 21 2010-12-29 12:11:06 PST
Comment on attachment 77591 [details] Patch r=me
Antti Koivisto
Comment 22 2010-12-29 12:19:27 PST
(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.
WebKit Commit Bot
Comment 23 2010-12-29 12:38:49 PST
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.
WebKit Commit Bot
Comment 24 2010-12-29 12:40:12 PST
Comment on attachment 77591 [details] Patch Clearing flags on attachment: 77591 Committed r74752: <http://trac.webkit.org/changeset/74752>
WebKit Commit Bot
Comment 25 2010-12-29 12:40:20 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 26 2010-12-29 13:29:00 PST
http://trac.webkit.org/changeset/74752 might have broken Leopard Intel Debug (Tests)
Note You need to log in before you can comment on or make changes to this bug.