Bug 22672 – ASSERT(m_table) when xhr.onabort creates another xhr or calls setTimeout

• Component: DOM
• Status: RESOLVED FIXED

[Dmitry Titov]

While stopping active dom objects we iterate over a set of them and call 'stop()'.
XmlHttpRequest, while stopping, synchronously invokes onabort event. If any other active object is created or removed, the iterator is invalidated underneath on the stack and ASSERT fires.
This stack shows how it happens (repro file is coming):

#0	0x0410f44f in WebCore::ScriptExecutionContext::createdActiveDOMObject at ScriptExecutionContext.cpp:160
#1	0x04106822 in WebCore::ActiveDOMObject::ActiveDOMObject at ActiveDOMObject.cpp:45
#2	0x03f014fc in WebCore::XMLHttpRequest::XMLHttpRequest at XMLHttpRequest.cpp:334
#3	0x03f01762 in WebCore::XMLHttpRequest::XMLHttpRequest at XMLHttpRequest.cpp:338
#4	0x0403ab1d in WebCore::XMLHttpRequest::create at XMLHttpRequest.h:41
#5	0x0403a8bb in constructXMLHttpRequest at JSXMLHttpRequestConstructor.cpp:46
#6	0x00581e0e in JSC::Interpreter::cti_op_construct_NotJSConstruct at Interpreter.cpp:5116
#7	0x0057b60c in JSC::Interpreter::retrieveCaller at Interpreter.cpp:4032
#8	0x0058052a in JSC::Interpreter::execute at Interpreter.cpp:1006
#9	0x004a2297 in JSC::JSFunction::call at JSFunction.cpp:82
#10	0x004a234f in JSC::call at CallData.cpp:39
#11	0x03f503a0 in WebCore::JSAbstractEventListener::handleEvent at JSEventListener.cpp:109
#12	0x03f01842 in WebCore::XMLHttpRequest::dispatchXMLHttpRequestProgressEvent at XMLHttpRequest.cpp:1387
#13	0x03f02abc in WebCore::XMLHttpRequest::dispatchAbortEvent at XMLHttpRequest.cpp:1397
#14	0x03f02c6f in WebCore::XMLHttpRequest::abort at XMLHttpRequest.cpp:872
#15	0x03f02cf5 in WebCore::XMLHttpRequest::stop at XMLHttpRequest.cpp:1429
#16	0x0410ece7 in WebCore::ScriptExecutionContext::stopActiveDOMObjects at ScriptExecutionContext.cpp:152
#17	0x03ab9456 in WebCore::FrameLoader::clear at FrameLoader.cpp:819
#18	0x03ab9727 in WebCore::FrameLoader::begin at FrameLoader.cpp:916
#19	0x03abbe5e in WebCore::FrameLoader::receivedFirstData at FrameLoader.cpp:866

[Dmitry Titov]

Created attachment 25762 [details]
reduced repro

simple file that causes assert.

[Dmitry Titov]

There could be fast and right solutions to this, I think. The fast one is to not use iterator in stopActiveDOMObjects but rather something like:
while(foo = activeObjects.begin()) {
foo->first->stop();
}
This can be an infinite loop so some counter and a limit could be used. Very hacky since it does not guarantee that objects will be stopped.

Better solution could include a way to deny a newly created object from becoming 'active'. For example, once SEC::stopActiveDOMObjects started to terminate active ones, external code can create XHR and call send() but xhr will be zombied from the start and not cause any listener callbacks. That feels right because if there is a concept of Active Object, the sequence of gaining 'active' status should be spelled out just as a sequence of loosing such status.

[Alexey Proskuryakov]

Another way to fix this could be undo the change that made stop() call abort() instead of internalAbort(). Even though the new behavior matches Firefox a little more closely, that's hardly important.

[Alexey Proskuryakov]

Created attachment 25770 [details]
proposed fix

[Alexey Proskuryakov]

Created attachment 25771 [details]
proposed fix

[Alexey Proskuryakov]

Created attachment 25772 [details]
proposed fix

Forgot to save ChangeLog when making the patch, sorry for the spam.

[Darin Adler]

Comment on attachment 25772 [details]
proposed fix

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 39023)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,36 @@
> [phone]  Alexey Proskuryakov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=22672
> +        ASSERT(m_table) when xhr.onabort creates another xhr or calls setTimeout
> +
> +        Test: http/tests/xmlhttprequest/send-on-abort.html
> +
> +        * dom/ScriptExecutionContext.cpp:
> +        (WebCore::ScriptExecutionContext::canSuspendActiveDOMObjects):
> +        (WebCore::ScriptExecutionContext::suspendActiveDOMObjects):
> +        (WebCore::ScriptExecutionContext::resumeActiveDOMObjects):
> +        (WebCore::ScriptExecutionContext::stopActiveDOMObjects):
> +        Add a comment explaining that ActiveDOMObject methods shouldn't execute arbitrary JS.
> +
> +        * xml/XMLHttpRequest.cpp: (WebCore::XMLHttpRequest::stop): Don't dispatch events. This
> +        reverts a recent change that made the behavior slightly closer to Firefox - but the
> +        compatibility effect should be very minor if any, and Firefox itself behaves inconsistently.
> +
> [phone]  Alexey Proskuryakov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * dom/ScriptExecutionContext.cpp:
> +        (WebCore::ScriptExecutionContext::canSuspendActiveDOMObjects):
> +        (WebCore::ScriptExecutionContext::suspendActiveDOMObjects):
> +        (WebCore::ScriptExecutionContext::resumeActiveDOMObjects):
> +        (WebCore::ScriptExecutionContext::stopActiveDOMObjects):
> +        * xml/XMLHttpRequest.cpp:
> +        (WebCore::XMLHttpRequest::stop):
> +

Double change log here.

> +        * http/tests/xmlhttprequest/abort-on-leaving-page-expected.txt: Removed.
> +        * http/tests/xmlhttprequest/abort-on-leaving-page.html: Removed.
> +        * http/tests/xmlhttprequest/frame-load-cancelled-abort-expected.txt:
> +        * http/tests/xmlhttprequest/resources/slow-response.pl: Removed.
> +        Stopping an XHR due to navigation no longer dispatches events.

Was there any way to change these tests to verify the new behavior, rather than removing them? Maybe that's pointless.

r=me

[Alexey Proskuryakov]

Committed revision 39025.

(In reply to comment #7)
> Was there any way to change these tests to verify the new behavior, rather than
> removing them? Maybe that's pointless.

I don't know how to - I changed internalAbort() to abort() previously exactly to be able to make this test.