Bug 11450

Summary: REGRESSION: XMLHttpRequest::didFinishLoading() should immediately return if the request has already been aborted
Product: WebKit Reporter: mitz
Component: Page LoadingAssignee: mitz
Status: VERIFIED FIXED    
Severity: Major CC: ap, beidson, ggaren
Priority: P1 Keywords: Regression
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.google.co.uk/search?q=WebKit
Attachments:
Description Flags
Patch + layout test
mjs: review+
Results from 3 different WebKits none

Description mitz 2006-10-29 13:47:58 PST
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
Comment 1 mitz 2006-10-31 13:22:04 PST
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...
Comment 2 mitz 2006-11-02 14:13:23 PST
*** Bug 11497 has been marked as a duplicate of this bug. ***
Comment 3 mitz 2006-11-02 14:14:17 PST
Bug 11497 contains steps to reproduce on Google Video.
Comment 4 mitz 2006-11-05 01:19:02 PST
(In reply to comment #1)
> One possible fix is[...] 

That was nonsense.
Comment 5 Alexey Proskuryakov 2006-11-06 21:34:08 PST
Fixed by Brady in r17637 (but this could still use a test case if we can make one). Closing for now.
Comment 6 Brady Eidson 2006-11-06 23:26:43 PST
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!
Comment 7 Brady Eidson 2006-11-07 23:54:11 PST
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...
Comment 8 Alexey Proskuryakov 2006-11-08 00:35:37 PST
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.
Comment 9 Brady Eidson 2006-11-08 01:43:30 PST
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()
Comment 10 Brady Eidson 2006-11-08 01:45:12 PST
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
Comment 11 Brady Eidson 2006-11-08 01:48:42 PST
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
Comment 12 Maciej Stachowiak 2006-11-08 02:25:16 PST
Comment on attachment 11420 [details]
Patch + layout test

r=me
Comment 13 Brady Eidson 2006-11-08 09:51:50 PST
Committed this in r17661 last night
Comment 14 Brady Eidson 2006-11-08 09:52:51 PST
Mitz please verify at your leisure ;)  (which, in my experience, will probably by like 3 seconds after I commit this comment to the bug)