Bug 83918 - REGRESSION (r100311): YummySoup app crashes when trying to print
: REGRESSION (r100311): YummySoup app crashes when trying to print
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Normal
Assigned To:
:
: InRadar
: 84665
:
  Show dependency treegraph
 
Reported: 2012-04-13 11:10 PST by
Modified: 2012-04-25 10:11 PST (History)


Attachments
patch (4.03 KB, patch)
2012-04-16 16:46 PST, Nate Chapin
ap: review+
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.08 MB, application/zip)
2012-04-16 19:05 PST, WebKit Review Bot
no flags Details
Fix test failures (11.41 KB, patch)
2012-04-23 12:58 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff
Fix by ensuring we don't call didFailToLoad() twice (3.32 KB, patch)
2012-04-24 16:51 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (4.96 KB, patch)
2012-04-25 09:29 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-13 11:10:44 PST
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>
------- Comment #1 From 2012-04-13 11:14:02 PST -------
(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 :)
------- Comment #2 From 2012-04-16 16:46:16 PST -------
Created an attachment (id=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 #3 From 2012-04-16 17:50:04 PST -------
(From update of attachment 137431 [details])
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 #4 From 2012-04-16 19:05:49 PST -------
(From update of attachment 137431 [details])
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
------- Comment #5 From 2012-04-16 19:05:55 PST -------
Created an attachment (id=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
------- Comment #6 From 2012-04-23 12:58:15 PST -------
Created an attachment (id=138402) [details]
Fix test failures

This patch is entirely different from the previous attempt, so asking for another review.
------- Comment #7 From 2012-04-23 13:34:01 PST -------
(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.

> 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.
------- Comment #8 From 2012-04-23 14:30:14 PST -------
(In reply to comment #7)
> (From update of attachment 138402 [details] [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).
------- Comment #9 From 2012-04-23 16:12:28 PST -------
(In reply to comment #7)
> (From update of attachment 138402 [details] [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.
------- Comment #10 From 2012-04-23 16:20:47 PST -------
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.
------- Comment #11 From 2012-04-23 16:23:59 PST -------
(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 #12 From 2012-04-23 17:14:22 PST -------
(From update of attachment 138402 [details])
Clearing flags on attachment: 138402

Committed r114965: <http://trac.webkit.org/changeset/114965>
------- Comment #13 From 2012-04-23 17:14:36 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #14 From 2012-04-23 18:19:41 PST -------
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
------- Comment #15 From 2012-04-23 18:20:41 PST -------
It looks like the resource load delegate no longer emits the didFinishLoading event.
------- Comment #16 From 2012-04-23 18:27:48 PST -------
Rolled out this commit in r114976.

Committed r114976: <http://trac.webkit.org/changeset/114976>
------- Comment #17 From 2012-04-24 16:51:15 PST -------
Created an attachment (id=138688) [details]
Fix by ensuring we don't call didFailToLoad() twice
------- Comment #18 From 2012-04-24 17:10:48 PST -------
(From update of attachment 138688 [details])
Please rename m_calledDidFinishLoad, or at least add a comment in header.
------- Comment #19 From 2012-04-25 09:29:31 PST -------
Created an attachment (id=138824) [details]
Patch for landing
------- Comment #20 From 2012-04-25 10:11:35 PST -------
(From update of attachment 138824 [details])
Clearing flags on attachment: 138824

Committed r115223: <http://trac.webkit.org/changeset/115223>
------- Comment #21 From 2012-04-25 10:11:49 PST -------
All reviewed patches have been landed.  Closing bug.