Bug 83918

(In reply to comment #0) > Steps to reproduce: > 1. Launch YummySoup! trial from http://hungryseacow.com/YummySoup!.zip > 2. Hit Cmd+P to open print dialog. > > The issue here is some bad code in SubresourceLoader::releaseResources(). Instead of releasing resources as promised, it does a number of unrelated things. Specifically, call to m_document->cachedResourceLoader()->loadDone() results in a delegate callback happening in an inconsistent state. When a client does something from this delegate call that touches this SubresourceLoader (e.g. starts a new load), all bets are off. > > Nate, would you be up to cleaning this up to fix the regression? > > <rdar://problem/11053870> As long as it doesn't need to be fixed today, absolutely :)

Created attachment 137431 [details] patch I'm using a URL with a bogus protocol to ensure the network stack passes us a didFail(), is there a more reliable way to do this?

Comment on attachment 137431 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=137431&action=review > Source/WebCore/loader/SubresourceLoader.cpp:318 > m_resource->stopLoading(); > m_documentLoader->removeSubresourceLoader(this); It would be nice to one day get rid of these non-"release" parts too. > LayoutTests/http/tests/xmlhttprequest/cancel-during-failure-crash.html:17 > +img.src = "force://didFail/fromnetworkstack"; Hmm, I didn't know that we were getting a failure from network stack in this case. Logically, WebKit may be doing sanity checks on protocols. Approaches I've used in the past were accessing an unused port on localhost (port 7 is one unlikely one to have listeners on), and doing an infinite redirect (which is useful when you need to do it conditionally, but that caused some problems with test performance on chromium IIRC). There may have been other tricks I've forgotten about - in particular, the infinite redirect trick in app cache directory was probably updated to something different.

Comment on attachment 137431 [details] patch Attachment 137431 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12419295 New failing tests: fast/dom/Window/window-function-frame-getter-precedence.html fast/harness/results.html fast/forms/access-key.html http/tests/misc/xslt-bad-import.html

Created attachment 137456 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 138402 [details] Fix test failures This patch is entirely different from the previous attempt, so asking for another review.

Comment on attachment 138402 [details] Fix test failures View in context: https://bugs.webkit.org/attachment.cgi?id=138402&action=review > LayoutTests/platform/mac/security/block-test-expected.txt:-4 > -http://255.255.255.255:7/test.jpg - willSendRequest <NSURLRequest URL http://255.255.255.255:7/test.jpg, main document URL block-test.html, http method GET> redirectResponse (null) Yeah, 255.255.255.255:7 is cool too. > Source/WebCore/loader/SubresourceLoader.cpp:280 > + if (reachedTerminalState()) > + return; Why is it OK to not call the base class in this case? > Source/WebCore/loader/SubresourceLoader.cpp:301 > + if (reachedTerminalState()) > + return; Ditto. > Source/WebCore/loader/SubresourceLoader.cpp:328 > m_resource->stopLoading(); I hope we can move this out in the future, too.

(In reply to comment #7) > (From update of attachment 138402 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138402&action=review > > Source/WebCore/loader/SubresourceLoader.cpp:280 > > + if (reachedTerminalState()) > > + return; > > Why is it OK to not call the base class in this case? The base classes of didFail() and didFinishLoading() both start with: if (m_cancelled) return; ASSERT(!m_reachedTerminalState); Also, for reachedTerminalState() to return true at this point, it must have been cancelled re-entrantly, so "m_requestCountTracker.clear();" and "m_document->cachedResourceLoader()->loadDone();" got called in willCancel(). > > > Source/WebCore/loader/SubresourceLoader.cpp:328 > > m_resource->stopLoading(); > > I hope we can move this out in the future, too. Yeah, I think it makes sense to move this into CachedResource in a later patch (e.g., have it get called by error() and finish() or something).

(In reply to comment #7) > (From update of attachment 138402 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138402&action=review > > > LayoutTests/platform/mac/security/block-test-expected.txt:-4 > > -http://255.255.255.255:7/test.jpg - willSendRequest <NSURLRequest URL http://255.255.255.255:7/test.jpg, main document URL block-test.html, http method GET> redirectResponse (null) > > Yeah, 255.255.255.255:7 is cool too. That's not actually from my test. This patch changes slight changes callback timings in the blocked port case, so these expected result changes reflect that. I didn't see anything concerning, but I'll wait until tomorrow morning to cq+ in case you see something that bothers you.

Yes, I understand that this is not from your test. What I was alluding to is that this address is a nice way to trigger load error even if there is something running on localhost:7.

(In reply to comment #10) > Yes, I understand that this is not from your test. What I was alluding to is that this address is a nice way to trigger load error even if there is something running on localhost:7. Ah...sorry for my misunderstanding.

Comment on attachment 138402 [details] Fix test failures Clearing flags on attachment: 138402 Committed r114965: <http://trac.webkit.org/changeset/114965>

All reviewed patches have been landed. Closing bug.

This commit appears to have caused four new test failures: fast/images/support-broken-image-delegate.html fast/loader/file-protocol-fragment.html webarchive/loading/cache-expired-subresource.html webarchive/loading/test-loading-archive-subresource-null-mimetype.html http://build.webkit.org/results/Lion%20Release%20(Tests)/r114971%20(7873)/results.html

It looks like the resource load delegate no longer emits the didFinishLoading event.

Rolled out this commit in r114976. Committed r114976: <http://trac.webkit.org/changeset/114976>

Created attachment 138688 [details] Fix by ensuring we don't call didFailToLoad() twice

Comment on attachment 138688 [details] Fix by ensuring we don't call didFailToLoad() twice Please rename m_calledDidFinishLoad, or at least add a comment in header.

Created attachment 138824 [details] Patch for landing

Comment on attachment 138824 [details] Patch for landing Clearing flags on attachment: 138824 Committed r115223: <http://trac.webkit.org/changeset/115223>

All reviewed patches have been landed. Closing bug.