[Webkit-unassigned] [Bug 106123] REGRESSION(r138222?): [Mac WK1] http/tests/appcache/main-resource-redirect.html asserts in WebFrameLoaderClient::dispatchDidFinishLoading

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106123





--- Comment #17 from Nate Chapin <japhet at chromium.org>  2013-01-08 16:32:01 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.