Bug 11450 – REGRESSION: XMLHttpRequest::didFinishLoading() should immediately return if the request has already been aborted

• Component: Page Loading
• Status: VERIFIED FIXED

ASSERTION FAILED: loader == m_loader
(WebCore/xml/xmlhttprequest.cpp:524 virtual void WebCore::XMLHttpRequest::didFinishLoading(WebCore::ResourceHandle*))

Thread 0 Crashed:
0   com.apple.WebCore        	0x027ffdb0 WebCore::XMLHttpRequest::didFinishLoading(WebCore::ResourceHandle*) + 116 (xmlhttprequest.cpp:524)
1   com.apple.WebCore        	0x02a71f70 WebCore::ResourceHandle::finishJobAndHandle(NSData*) + 132 (ResourceHandleMac.mm:150)
2   com.apple.WebCore        	0x02a71fbc WebCore::ResourceHandle::reportError() + 48 (ResourceHandleMac.mm:157)
3   com.apple.WebCore        	0x02a68b80 WebCore::SubresourceLoader::didCancel(NSError*) + 272 (WebSubresourceLoader.mm:204)
4   com.apple.WebCore        	0x02a62e98 WebCore::WebResourceLoader::cancel(NSError*) + 144 (WebLoader.mm:415)
5   com.apple.WebCore        	0x02a61c94 WebCore::cancelAll(WTF::HashSet<WTF::RefPtr<WebCore::WebResourceLoader>, WTF::PtrHash<WTF::RefPtr<WebCore::WebResourceLoader> >, WTF::HashTraits<WTF::RefPtr<WebCore::WebResourceLoader> > > const&) + 136 (FrameLoader.mm:82)
6   com.apple.WebCore        	0x02a61d1c WebCore::FrameLoader::stopLoadingSubresources() + 40 (FrameLoader.mm:446)
7   com.apple.WebCore        	0x02a55678 WebCore::DocumentLoader::stopLoading() + 312 (WebDocumentLoader.mm:303)
8   com.apple.WebCore        	0x02a5df4c WebCore::FrameLoader::stopLoading() + 268 (FrameLoader.mm:474)
9   com.apple.WebCore        	0x02a5e4b4 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(NSURLRequest*, WTF::PassRefPtr<WebCore::FormState>) + 592 (FrameLoader.mm:1485)
10  com.apple.WebCore        	0x02a5e68c WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy(void*, NSURLRequest*, WTF::PassRefPtr<WebCore::FormState>) + 76 (FrameLoader.mm:1446)
11  com.apple.WebCore        	0x02a5d388 WebCore::PolicyCheck::call() + 144 (FrameLoader.mm:2078)
12  com.apple.WebCore        	0x02a5dcb8 WebCore::FrameLoader::continueAfterNavigationPolicy(WebCore::PolicyAction) + 364 (FrameLoader.mm:1427)
13  com.apple.WebKit         	0x010d7150 WebFrameLoaderClient::receivedPolicyDecison(WebCore::PolicyAction) + 392 (WebFrameLoaderClient.mm:1165)
14  com.apple.WebKit         	0x010d77f8 -[WebFramePolicyListener receivedPolicyDecision:] + 140 (WebFrameLoaderClient.mm:1210)
15  com.apple.WebKit         	0x010d6f60 -[WebFramePolicyListener use] + 64 (WebFrameLoaderClient.mm:1226)
16  libobjc.A.dylib          	0x90a441f4 objc_msgSendv + 180
17  com.apple.Foundation     	0x9295cc88 -[NSInvocation invoke] + 944
18  com.apple.Foundation     	0x9295d238 -[NSInvocation invokeWithTarget:] + 64
19  com.apple.WebKit         	0x01091804 -[_WebSafeForwarder forwardInvocation:] + 632 (WebView.mm:1626)
20  com.apple.Foundation     	0x92955034 -[NSObject(NSForwardInvocation) forward::] + 408
21  libobjc.A.dylib          	0x90a440b0 _objc_msgForward + 176
22  com.apple.WebKit         	0x010d73ec WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(void (WebCore::FrameLoader::*)(WebCore::PolicyAction), NSDictionary*, NSURLRequest*) + 204 (WebFrameLoaderClient.mm:666)
23  com.apple.WebCore        	0x02a5d958 WebCore::FrameLoader::checkNavigationPolicy(NSURLRequest*, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, void (*)(void*, NSURLRequest*, WTF::PassRefPtr<WebCore::FormState>), void*) + 692 (FrameLoader.mm:1403)
24  com.apple.WebCore        	0x02a5f708 WebCore::FrameLoader::load(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>) + 476 (FrameLoader.mm:359)
25  com.apple.WebCore        	0x02a5fde8 WebCore::FrameLoader::load(NSURLRequest*, NSDictionary*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>) + 384 (FrameLoader.mm:310)
26  com.apple.WebCore        	0x02a60e8c WebCore::FrameLoader::load(NSURL*, WebCore::String const&, WebCore::FrameLoadType, WebCore::String const&, NSEvent*, WebCore::Element*, WTF::HashMap<WebCore::String, WebCore::String, WTF::StrHash<WebCore::String>, WTF::HashTraits<WebCore::String>, WTF::HashTraits<WebCore::String> > const&) + 1392 (FrameLoader.mm:262)
27  com.apple.WebCore        	0x02a61248 WebCore::FrameLoader::load(WebCore::FrameLoadRequest const&, bool, NSEvent*, WebCore::Element*, WTF::HashMap<WebCore::String, WebCore::String, WTF::StrHash<WebCore::String>, WTF::HashTraits<WebCore::String>, WTF::StrHash<WebCore::String> > const&) + 716 (FrameLoader.mm:196)
28  com.apple.WebCore        	0x026ed914 WebCore::FrameMac::urlSelected(WebCore::FrameLoadRequest const&, WebCore::Event const*) + 236 (FrameMac.mm:217)
29  com.apple.WebCore        	0x026d854c WebCore::Frame::urlSelected(WebCore::ResourceRequest const&, WebCore::String const&, WebCore::Event const*, bool) + 752 (Frame.cpp:292)
30  com.apple.WebCore        	0x029332ac WebCore::HTMLAnchorElement::defaultEventHandler(WebCore::Event*) + 2188 (HTMLAnchorElement.cpp:209)
31  com.apple.WebCore        	0x0289121c WebCore::EventTargetNode::dispatchGenericEvent(WTF::PassRefPtr<WebCore::Event>, int&, bool) + 2196 (EventTargetNode.cpp:256)
32  com.apple.WebCore        	0x02891cf8 WebCore::EventTargetNode::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, int&, bool) + 348 (EventTargetNode.cpp:292)
33  com.apple.WebCore        	0x028926c0 WebCore::EventTargetNode::dispatchMouseEvent(WebCore::AtomicString const&, int, int, int, int, int, int, bool, bool, bool, bool, bool, WebCore::Node*) + 580 (EventTargetNode.cpp:418)
34  com.apple.WebCore        	0x02892bf8 WebCore::EventTargetNode::dispatchMouseEvent(WebCore::PlatformMouseEvent const&, WebCore::AtomicString const&, int, WebCore::Node*) + 392 (EventTargetNode.cpp:376)
35  com.apple.WebCore        	0x026f0d14 WebCore::FrameView::dispatchMouseEvent(WebCore::AtomicString const&, WebCore::Node*, bool, int, WebCore::PlatformMouseEvent const&, bool) + 760 (FrameView.cpp:1192)
36  com.apple.WebCore        	0x026f1278 WebCore::FrameView::handleMouseReleaseEvent(WebCore::PlatformMouseEvent const&) + 700 (FrameView.cpp:901)
37  com.apple.WebCore        	0x026dd27c WebCore::FrameMac::mouseUp(NSEvent*) + 536 (FrameMac.mm:1854)
38  com.apple.WebKit         	0x0105e7d4 -[WebHTMLView mouseUp:] + 344 (WebHTMLView.m:3052)
39  com.apple.AppKit         	0x93767900 -[NSWindow sendEvent:] + 4728
40  com.apple.Safari         	0x00021734 0x1000 + 132916
41  com.apple.AppKit         	0x937108d4 -[NSApplication sendEvent:] + 4172
42  com.apple.Safari         	0x00021238 0x1000 + 131640
43  com.apple.AppKit         	0x93707d10 -[NSApplication run] + 508
44  com.apple.AppKit         	0x937f887c NSApplicationMain + 452
45  com.apple.Safari         	0x0005c77c 0x1000 + 374652
46  com.apple.Safari         	0x0005c624 0x1000 + 374308

The XMLHttpRequest is abort()ed under Frame::stopLoading(). It's m_loader is then set to 0, but the SubresourceLoader remains in the FrameLoader's m_subresourceLoaders set, so when FrameLoader::stopLoadingSubresources() is called, the SubresourceLoader is canceled and XMLHttpRequest::didFinishLoading() is called.

One possible fix is for XMLHttpRequest::abort() to get call the obtain the SubresourceLoader's FrameLoader and call its removeSubresourceLoader() method. However, currently ResourceLoader::frameLoader() is Mac-only...

*** Bug 11497 has been marked as a duplicate of this bug. ***

Bug 11497 contains steps to reproduce on Google Video.

(In reply to comment #1)
> One possible fix is[...] 

That was nonsense.

[Alexey Proskuryakov]

Fixed by Brady in r17637 (but this could still use a test case if we can make one). Closing for now.

[Brady Eidson]

Its true I fixed the assertion failure earlier today, but it was a bandaid.  I'm about to propose a much better bandaid (notice, still a bandaid)  stay tuned!

[Brady Eidson]

Mitz and I think that if XMLHttpRequest::didFinishLoading() is called after an abort that the method should immediately bail and not do any state change (and therefore notifications)

With the fix I have and the layout test I've constructed, that is the case.  However, running my layout test with shipping safari shows we *do* get the state change notifcation from 1 to 4 (loading to complete)

It may be a problem in my layout-testing methodology, I'll double check that now...

[Alexey Proskuryakov]

I worries me that didFinishLoading() is even called after an abort() - could this cause problems when the XMLHttpRequest object is reused? I.e., something like
req.send(...);
req.abort();
req.open(...);
req.send(...);
// didFinishLoading() gets called twice.

[Brady Eidson]

didFinishedLoading() seems to be called after the abort because the frame is being torn down.

The process of nuking the frame calls abort() then didFinishLoading()

[Brady Eidson]

Created attachment 11420 [details]
Patch + layout test

This a shot...  the code fix is pretty simple.  I will have commentary to follow, though, on whether its the right result and I'll ask Maciej to take a look

[Brady Eidson]

Created attachment 11421 [details]
Results from 3 different WebKits

Basically, we have three different possible results for my layout test - ToT was already different from shipping WebKit, but my code change makes it different still.  I'd like Maciej to take a look at this

[Maciej Stachowiak]

Comment on attachment 11420 [details]
Patch + layout test

r=me

[Brady Eidson]

Committed this in r17661 last night

[Brady Eidson]

Mitz please verify at your leisure ;)  (which, in my experience, will probably by like 3 seconds after I commit this comment to the bug)