Bug 105044

Summary: REGRESSION (r137607): Cannot download files, stuck in “Preparing to download”
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Page LoadingAssignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, beidson, cdumez, japhet, naginenis, svetloslav, svillar, tmpsantos, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ap: review+
Fix the regression none

Alexey Proskuryakov
Reported 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>
Attachments
Patch (1.89 KB, patch)
2012-12-14 12:35 PST, Anders Carlsson 		ap: review+
DetailsFormatted DiffDiff
Fix the regression (2.70 KB, patch)
2012-12-17 08:34 PST, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 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.
Anders Carlsson
Comment 2 2012-12-14 12:34:28 PST
*** Bug 105050 has been marked as a duplicate of this bug. ***
Anders Carlsson
Comment 3 2012-12-14 12:35:49 PST
Created attachment 179515 [details] Patch
Alexey Proskuryakov
Comment 4 2012-12-14 12:43:59 PST
Comment on attachment 179515 [details] Patch Please keep the bug open for Nate to investigate.
Alexey Proskuryakov
Comment 5 2012-12-14 12:46:06 PST
*** Bug 105033 has been marked as a duplicate of this bug. ***
Anders Carlsson
Comment 6 2012-12-14 12:50:07 PST
Committed r137763: <http://trac.webkit.org/changeset/137763>
Thiago Marcos P. Santos
Comment 7 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.
Nate Chapin
Comment 8 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.
Sergio Villar Senin
Comment 9 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?
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2012-12-18 05:06:02 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.