Bug 51067 - Assertion failure: element->inDocument() in AsyncScriptRunner::executeScriptSoon()
Summary: Assertion failure: element->inDocument() in AsyncScriptRunner::executeScriptS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-14 15:49 PST by Alexey Proskuryakov
Modified: 2010-12-29 13:29 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 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
Comment 1 Tony Gentilcore 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.
Comment 2 Tony Gentilcore 2010-12-28 11:22:36 PST
Created attachment 77564 [details]
Patch
Comment 3 Tony Gentilcore 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);
}
Comment 4 Eric Seidel (no email) 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.
Comment 5 Tony Gentilcore 2010-12-28 11:38:37 PST
Created attachment 77566 [details]
Patch for landing
Comment 6 Alexey Proskuryakov 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?
Comment 7 Alexey Proskuryakov 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?
Comment 8 WebKit Commit Bot 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
Comment 9 Eric Seidel (no email) 2010-12-28 13:36:04 PST
I <3 the commit-queue.
Comment 10 Tony Gentilcore 2010-12-28 16:32:33 PST
Created attachment 77590 [details]
Patch
Comment 11 Tony Gentilcore 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.
Comment 12 Ryosuke Niwa 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?
Comment 13 Eric Seidel (no email) 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.
Comment 14 Eric Seidel (no email) 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.
Comment 15 Tony Gentilcore 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.
Comment 16 Tony Gentilcore 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.
Comment 17 Tony Gentilcore 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).
Comment 18 Tony Gentilcore 2010-12-28 16:58:36 PST
Created attachment 77591 [details]
Patch
Comment 19 Tony Gentilcore 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.
Comment 20 Tony Gentilcore 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
Comment 21 Antti Koivisto 2010-12-29 12:11:06 PST
Comment on attachment 77591 [details]
Patch

r=me
Comment 22 Antti Koivisto 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.
Comment 23 WebKit Commit Bot 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.
Comment 24 WebKit Commit Bot 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>
Comment 25 WebKit Commit Bot 2010-12-29 12:40:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Review Bot 2010-12-29 13:29:00 PST
http://trac.webkit.org/changeset/74752 might have broken Leopard Intel Debug (Tests)