Bug 105044 - REGRESSION (r137607): Cannot download files, stuck in “Preparing to download”
Summary: REGRESSION (r137607): Cannot download files, stuck in “Preparing to download”
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords: InRadar
: 105033 105050 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-12-14 11:55 PST by Alexey Proskuryakov
Modified: 2012-12-18 05:06 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.89 KB, patch)
2012-12-14 12:35 PST, Anders Carlsson
ap: review+
Details | Formatted Diff | Diff
Fix the regression (2.70 KB, patch)
2012-12-17 08:34 PST, 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-12-14 11:55:47 PST
Downloads no longer work in Safari, and probably in other WebKit based browsers too.

Steps to reproduce: go to nightlies.webkit.org, click on a link to download a nightly.

<rdar://problem/12239483>
Comment 1 Alexey Proskuryakov 2012-12-14 11:57:25 PST
What happens here is that we are getting a ResourceHandle::cancel() call now.

Anders is going to address this symptom by making ResourceHandle::cancel() a no-op once download has been started, but the change in ResourceLoader behavior is unexpected, and possibly wrong.
Comment 2 Anders Carlsson 2012-12-14 12:34:28 PST
*** Bug 105050 has been marked as a duplicate of this bug. ***
Comment 3 Anders Carlsson 2012-12-14 12:35:49 PST
Created attachment 179515 [details]
Patch
Comment 4 Alexey Proskuryakov 2012-12-14 12:43:59 PST
Comment on attachment 179515 [details]
Patch

Please keep the bug open for Nate to investigate.
Comment 5 Alexey Proskuryakov 2012-12-14 12:46:06 PST
*** Bug 105033 has been marked as a duplicate of this bug. ***
Comment 6 Anders Carlsson 2012-12-14 12:50:07 PST
Committed r137763: <http://trac.webkit.org/changeset/137763>
Comment 7 Thiago Marcos P. Santos 2012-12-17 06:19:57 PST
(In reply to comment #6)
> Committed r137763: <http://trac.webkit.org/changeset/137763>

Do you have a webkit-wise fix for this? I'm having the same issue on EFL since r137607.
Comment 8 Nate Chapin 2012-12-17 08:34:00 PST
Created attachment 179748 [details]
Fix the regression

It appears that in r137607 we call DocumentLoader::mainReceivedError(), which now calls MainResourceLoader::cancel(), which cancels the CachedResource load, which cancels the ResourceLoader, which cancels the ResourceHandle.

Calling directly into ResourceLoader::didFail() instead is a bit of a layering violation, but it emulates the old behavior quite well.  Strictly speaking, it isn't necessary to make downloads work, but if we don't, the loader objects (ResourceLoader, CachedResource, MainResourceLoader) will be left alive but useless until the associated DocumentLoader is detached.
Comment 9 Sergio Villar Senin 2012-12-18 01:30:44 PST
(In reply to comment #8)
> Created an attachment (id=179748) [details]
> Fix the regression
> 
> It appears that in r137607 we call DocumentLoader::mainReceivedError(), which now calls MainResourceLoader::cancel(), which cancels the CachedResource load, which cancels the ResourceLoader, which cancels the ResourceHandle.
> 
> Calling directly into ResourceLoader::didFail() instead is a bit of a layering violation, but it emulates the old behavior quite well.  Strictly speaking, it isn't necessary to make downloads work, but if we don't, the loader objects (ResourceLoader, CachedResource, MainResourceLoader) will be left alive but useless until the associated DocumentLoader is detached.

LGTM, BTW doesn't this regression mean that we lack some test?
Comment 10 WebKit Review Bot 2012-12-18 05:05:56 PST
Comment on attachment 179748 [details]
Fix the regression

Clearing flags on attachment: 179748

Committed r138012: <http://trac.webkit.org/changeset/138012>
Comment 11 WebKit Review Bot 2012-12-18 05:06:02 PST
All reviewed patches have been landed.  Closing bug.