Bug 83918 – REGRESSION (r100311): YummySoup app crashes when trying to print

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Alexey Proskuryakov]

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>

[Nate Chapin]

(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 :)

[Nate Chapin]

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?

[Alexey Proskuryakov]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nate Chapin]

Created attachment 138402 [details]
Fix test failures

This patch is entirely different from the previous attempt, so asking for another review.

[Alexey Proskuryakov]

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.

[Nate Chapin]

(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).

[Nate Chapin]

(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.

[Alexey Proskuryakov]

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.

[Nate Chapin]

(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.

[WebKit Review Bot]

Comment on attachment 138402 [details]
Fix test failures

Clearing flags on attachment: 138402

Committed r114965: <http://trac.webkit.org/changeset/114965>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

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

[Jer Noble]

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

[Jer Noble]

Rolled out this commit in r114976.

Committed r114976: <http://trac.webkit.org/changeset/114976>

[Nate Chapin]

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

[Alexey Proskuryakov]

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.

[Nate Chapin]

Created attachment 138824 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 138824 [details]
Patch for landing

Clearing flags on attachment: 138824

Committed r115223: <http://trac.webkit.org/changeset/115223>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.