Summary: | REGRESSION: XMLHttpRequest::didFinishLoading() should immediately return if the request has already been aborted | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||
Component: | Page Loading | Assignee: | 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
mitz
2006-10-29 13:47:58 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... *** Bug 11497 has been marked as a duplicate of this bug. *** (In reply to comment #1) > One possible fix is[...] That was nonsense. Fixed by Brady in r17637 (but this could still use a test case if we can make one). Closing for now. 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! 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... 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. 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() 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
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 on attachment 11420 [details]
Patch + layout test
r=me
Committed this in r17661 last night Mitz please verify at your leisure ;) (which, in my experience, will probably by like 3 seconds after I commit this comment to the bug) |