WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Patch
(4.28 KB, patch)
2010-12-28 11:22 PST
,
Tony Gentilcore
no flags
Details
Formatted Diff
Diff
Patch for landing
(4.24 KB, patch)
2010-12-28 11:38 PST
,
Tony Gentilcore
no flags
Details
Formatted Diff
Diff
Patch
(8.39 KB, patch)
2010-12-28 16:32 PST
,
Tony Gentilcore
no flags
Details
Formatted Diff
Diff
Patch
(7.55 KB, patch)
2010-12-28 16:58 PST
,
Tony Gentilcore
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 77564
[details]
Patch
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
Created
attachment 77590
[details]
Patch
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
Created
attachment 77591
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug