Summary: | REGRESSION(r138222?): [Mac WK1] http/tests/appcache/main-resource-redirect.html asserts in WebFrameLoaderClient::dispatchDidFinishLoading | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
Component: | Page Loading | Assignee: | Nate Chapin <japhet> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, beidson, cdumez, cgarcia, eric.carlson, japhet, webkit-bug-importer, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 105591 | ||||||||||||||
Bug Blocks: | 106137 | ||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2013-01-04 12:05:07 PST
It’s hard to tell whether this is a recent regression or something that existed for a long time. Oh, now I see what’s happening. http://trac.webkit.org/changeset/138782 tried to fix this crash but it didn’t. This is only happening in WebKit1. Updated test expectations in http://trac.webkit.org/changeset/138828. Turned out that this is an assertion failure: - (void)_removeObjectForIdentifier:(unsigned long)identifier { ASSERT(_private->identifierMap.contains(identifier)); << this guy. _private->identifierMap.remove(identifier); // If the identifier map is now empty it means we're no longer loading anything // and we should release the web view. Autorelease rather than release in order to // avoid re-entering this method beneath -dealloc with the same identifier. <rdar://problem/10523721> if (_private->identifierMap.isEmpty()) WebCFAutorelease(self); } (In reply to comment #3) > Oh, now I see what’s happening. http://trac.webkit.org/changeset/138782 tried to fix this crash but it didn’t. Not exactly, they are actually different bugs. Bug #105591 fixed an assertion when the resource identifier is 0. The problem now is that for some reason, dispatchDidFailLoading() is called for a resource, but the load doesn't actually finish and dispatchDidReceiveResponse(), dispatchDidReceiveData() and dispatchDidFinishLoading() are called for the same resource after the dispatchDidFailLoading(). This is of course not expected, Mac removes the object from the map in both DidFailLoading and DidFinishLoading. Created attachment 181433 [details]
Patch
The problem is that we are clearing the resource before continuing loading from the substitute data, which cancels the load.
Apparently we can also get a timeout. http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r138916%20(5487)/http/tests/appcache/main-resource-redirect-pretty-diff.html Nate, Brady, who should review this? Comment on attachment 181433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181433&action=review This presumably should remove main-resource-redirect.html from the relevant TestExpectations files. > Source/WebCore/loader/MainResourceLoader.cpp:151 > + if (m_resource) { > + m_resource->setLoading(false); > + m_resource->stopLoading(); > + } > +} This is better than anything I've come up with in that it actually works, but it's still pretty hacky. I guess it's ok, though. Created attachment 181668 [details]
Unskip the test
Updated patch to unskip the test
Comment on attachment 181668 [details] Unskip the test Attachment 181668 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15763049 New failing tests: http/tests/appcache/manifest-containing-itself.html Comment on attachment 181668 [details] Unskip the test Attachment 181668 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15757070 New failing tests: http/tests/appcache/manifest-containing-itself.html Created attachment 181725 [details]
Fix by preventing ResourceLoader from sending callbacks
The previous fix doesn't work because CachedResource::stopLoading() doesn't actually kill stop the ResourceLoader (renaming it is on my list of things to do).
It appears we can get the right behavior by temporarily suppressing resource load callbacks, which will prevent WebFrameLoaderClient from receiving 2 end-of-load callbacks.
Comment on attachment 181725 [details] Fix by preventing ResourceLoader from sending callbacks View in context: https://bugs.webkit.org/attachment.cgi?id=181725&action=review > Source/WebCore/loader/MainResourceLoader.cpp:185 > + // We need to remove our reference to the CachedResource (which will probably cancel its underlying ResourceLoader) Why is the cancellation just "probable"? If the load is not canceled now, will it be canceled later, outside the temporary DoNotSendCallbacks mode? > Source/WebCore/loader/MainResourceLoader.cpp:192 > + if (resourceLoader) > + resourceLoader->setSendCallbackPolicy(DoNotSendCallbacks); > clearResource(); > + resourceLoader->setSendCallbackPolicy(SendCallbacks); It looks wrong to have a null check the first time we dereference resourceLoader, but not the second time. If it's actually possible to not have a resourceLoader here, please add a regression test for this case. Also, is this expected to reset policy to original value? Can we assert that SendCallbacks is actually the original value? > Source/WebCore/ChangeLog:14 > + (WebCore::ResourceLoader::sendCallbackPolicy): Renamed from shouldSendResourceLoadCallbacks(), returns an enum value instead of a bool. I'm not sure why this is an improvement. A boolean gives the needed answer more directly, as evidenced by how you had to modify the call site. (In reply to comment #16) > (From update of attachment 181725 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181725&action=review > > > Source/WebCore/loader/MainResourceLoader.cpp:185 > > + // We need to remove our reference to the CachedResource (which will probably cancel its underlying ResourceLoader) > > Why is the cancellation just "probable"? If the load is not canceled now, will it be canceled later, outside the temporary DoNotSendCallbacks mode? There is a tiny chance that multiple iframes will be loading the same src at the same exact time. In that case, the CachedResource will have multiple clients, so calling clearResource() won't actually trigger a cancellation of the ResourceLoader via CachedRawResource::cancelIfNotFinishing(). > > > Source/WebCore/loader/MainResourceLoader.cpp:192 > > + if (resourceLoader) > > + resourceLoader->setSendCallbackPolicy(DoNotSendCallbacks); > > clearResource(); > > + resourceLoader->setSendCallbackPolicy(SendCallbacks); > > It looks wrong to have a null check the first time we dereference resourceLoader, but not the second time. If it's actually possible to not have a resourceLoader here, please add a regression test for this case. Doh, will fix. > > Also, is this expected to reset policy to original value? Can we assert that SendCallbacks is actually the original value? Will do. > > > Source/WebCore/ChangeLog:14 > > + (WebCore::ResourceLoader::sendCallbackPolicy): Renamed from shouldSendResourceLoadCallbacks(), returns an enum value instead of a bool. > > I'm not sure why this is an improvement. A boolean gives the needed answer more directly, as evidenced by how you had to modify the call site. I was thinking that this was canonicalizing things, based on some feedback on a review a couple weeks ago. I'm totally fine with removing it. > There is a tiny chance that multiple iframes will be loading the same src at the same exact time. In that case, the CachedResource will have multiple clients, so calling clearResource() won't actually trigger a cancellation of the ResourceLoader via CachedRawResource::cancelIfNotFinishing().
This is probably worth explaining in a comment more directly. As well as how the switch to substitute data will happen in this case.
There is a risk associated with such detailed comments, as they tend to become wrong and misleading very quickly. But a "probably" clause in a comment is quite annoying, too.
Created attachment 181802 [details]
Address ap's comments
Comment on attachment 181802 [details] Address ap's comments View in context: https://bugs.webkit.org/attachment.cgi?id=181802&action=review > Source/WebCore/loader/MainResourceLoader.cpp:193 > + if (resourceLoader) I asked Nate on IRC, and he doesn't know if this can actually happen in practice. It's very confusing to have unnecessary null checks just in case, but given the urgency, I guess I can grudgingly say r=me. > Source/WebCore/loader/ResourceLoader.h:143 > + void setSendCallbackPolicy(SendCallbackPolicy sendLoadCallbacks) { m_options.sendLoadCallbacks = sendLoadCallbacks; } Extra space before '{'. Created attachment 181808 [details]
Patch for landing
Comment on attachment 181808 [details] Patch for landing Clearing flags on attachment: 181808 Committed r139150: <http://trac.webkit.org/changeset/139150> All reviewed patches have been landed. Closing bug. |