Bug 51067 – Assertion failure: element->inDocument() in AsyncScriptRunner::executeScriptSoon()

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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

[Tony Gentilcore]

This might be a dup of https://bugs.webkit.org/show_bug.cgi?id=49935, I'll look into this.

[Tony Gentilcore]

Created attachment 77564 [details]
Patch

[Tony Gentilcore]

> 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 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]

Created attachment 77566 [details]
Patch for landing

[Alexey Proskuryakov]

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]

> 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 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)]

I <3 the commit-queue.

[Tony Gentilcore]

Created attachment 77590 [details]
Patch

[Tony Gentilcore]

> 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]

(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 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)]

I don't know if you'd have to store the cached Document pointer in a RefPtr or not.

[Tony Gentilcore]

> 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]

(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]

> 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]

Created attachment 77591 [details]
Patch

[Tony Gentilcore]

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]

(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 on attachment 77591 [details]
Patch

r=me

[Antti Koivisto]

(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]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 77591 [details]
Patch

Clearing flags on attachment: 77591

Committed r74752: <http://trac.webkit.org/changeset/74752>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74752 might have broken Leopard Intel Debug (Tests)