Bug 83918 - REGRESSION (r100311): YummySoup app crashes when trying to print
: REGRESSION (r100311): YummySoup app crashes when trying to print
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Page Loading
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Normal
Assigned To: Nate Chapin
: InRadar
Depends on: 84665
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 11:10 PDT by Alexey Proskuryakov
Modified: 2012-04-25 10:11 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2012-04-13 11:10:44 PDT
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 Nate Chapin 2012-04-13 11:14:02 PDT
(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 Nate Chapin 2012-04-16 16:46:16 PDT
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 3 Alexey Proskuryakov 2012-04-16 17:50:04 PDT
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 4 WebKit Review Bot 2012-04-16 19:05:49 PDT
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
Comment 5 WebKit Review Bot 2012-04-16 19:05:55 PDT
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
Comment 6 Nate Chapin 2012-04-23 12:58:15 PDT
Created attachment 138402 [details]
Fix test failures

This patch is entirely different from the previous attempt, so asking for another review.
Comment 7 Alexey Proskuryakov 2012-04-23 13:34:01 PDT
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.
Comment 8 Nate Chapin 2012-04-23 14:30:14 PDT
(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).
Comment 9 Nate Chapin 2012-04-23 16:12:28 PDT
(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.
Comment 10 Alexey Proskuryakov 2012-04-23 16:20:47 PDT
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 Nate Chapin 2012-04-23 16:23:59 PDT
(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 WebKit Review Bot 2012-04-23 17:14:22 PDT
Comment on attachment 138402 [details]
Fix test failures

Clearing flags on attachment: 138402

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

Committed r114976: <http://trac.webkit.org/changeset/114976>
Comment 17 Nate Chapin 2012-04-24 16:51:15 PDT
Created attachment 138688 [details]
Fix by ensuring we don't call didFailToLoad() twice
Comment 18 Alexey Proskuryakov 2012-04-24 17:10:48 PDT
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.
Comment 19 Nate Chapin 2012-04-25 09:29:31 PDT
Created attachment 138824 [details]
Patch for landing
Comment 20 WebKit Review Bot 2012-04-25 10:11:35 PDT
Comment on attachment 138824 [details]
Patch for landing

Clearing flags on attachment: 138824

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