Bug 156099 - <a download> does not support Blob URLs
Summary: <a download> does not support Blob URLs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://jsfiddle.net/cW7W5/1579/
Keywords: InRadar
: 156180 (view as bug list)
Depends on: 156057 156180 162541 162543
Blocks: 156056 162535
  Show dependency treegraph
 
Reported: 2016-03-31 23:54 PDT by Brent Fulgham
Modified: 2016-09-25 20:07 PDT (History)
13 users (show)

See Also:


Attachments
WIP patch (24.71 KB, patch)
2016-09-23 20:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (24.72 KB, patch)
2016-09-23 20:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (with tests) (29.46 KB, patch)
2016-09-23 22:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (with more tests) (34.06 KB, patch)
2016-09-23 23:14 PDT, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2 (deleted)
2016-09-24 00:39 PDT, Build Bot
no flags Details
WIP Patch (with more tests) (32.64 KB, patch)
2016-09-24 09:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.25 KB, patch)
2016-09-24 13:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.65 KB, patch)
2016-09-24 19:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-03-31 23:54:45 PDT
The current implementation of <a download> fails to handle Blob URLs properly.

Tested by: http/tests/security/anchor-download-allow-blob.html
Comment 1 Brent Fulgham 2016-04-03 23:50:39 PDT
This is happening because the following fails:

void NavigationState::NavigationClient::decidePolicyForNavigationAction:

(Line 303):
            if ([NSURLConnection canHandleRequest:nsURLRequest.get()]) {

Need to see how Blob URLs are handled elsewhere.
Comment 2 Radar WebKit Bug Importer 2016-04-04 13:20:42 PDT
<rdar://problem/25535520>
Comment 3 Brady Eidson 2016-04-05 11:36:51 PDT
(In reply to comment #1)
> This is happening because the following fails:
> 
> void NavigationState::NavigationClient::decidePolicyForNavigationAction:
> 
> (Line 303):
>             if ([NSURLConnection canHandleRequest:nsURLRequest.get()]) {
> 
> Need to see how Blob URLs are handled elsewhere.

I don't think this is the problem. A known download should never be a naviagtion in the first place.
Comment 4 Chris Dumez 2016-09-23 18:31:45 PDT
I have a WIP patch that seems to work. I'll get it ready for review.
Comment 5 Chris Dumez 2016-09-23 20:26:13 PDT
Created attachment 289736 [details]
WIP patch
Comment 6 Chris Dumez 2016-09-23 20:29:45 PDT
Created attachment 289737 [details]
WIP Patch
Comment 7 Chris Dumez 2016-09-23 20:49:21 PDT
Test page:
http://jsfiddle.net/cW7W5/1579/
Comment 8 Chris Dumez 2016-09-23 22:48:21 PDT
Created attachment 289739 [details]
WIP Patch (with tests)
Comment 9 Chris Dumez 2016-09-23 23:14:42 PDT
Created attachment 289740 [details]
WIP Patch (with more tests)
Comment 10 Build Bot 2016-09-24 00:39:02 PDT
Comment on attachment 289740 [details]
WIP Patch (with more tests)

Attachment 289740 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2135745

New failing tests:
http/tests/security/anchor-download-allow-blob.html
fast/dom/HTMLAnchorElement/anchor-download-unset.html
fast/dom/HTMLAnchorElement/anchor-nodownload.html
Comment 11 Build Bot 2016-09-24 00:39:07 PDT
Created attachment 289741 [details]
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 12 Chris Dumez 2016-09-24 09:25:22 PDT
Created attachment 289745 [details]
WIP Patch (with more tests)
Comment 13 Chris Dumez 2016-09-24 13:42:20 PDT
Created attachment 289756 [details]
Patch
Comment 14 Darin Adler 2016-09-24 17:54:06 PDT
Comment on attachment 289756 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289756&action=review

> Source/WebKit2/NetworkProcess/Downloads/BlobDownloadClient.h:45
> +    // ResourceHandleClient
> +    void didReceiveResponse(WebCore::ResourceHandle*, WebCore::ResourceResponse&&) final;
> +    void didReceiveBuffer(WebCore::ResourceHandle*, Ref<WebCore::SharedBuffer>&&, int reportedEncodedDataLength) final;
> +    void didFinishLoading(WebCore::ResourceHandle*, double finishTime) final;
> +    void didFail(WebCore::ResourceHandle*, const WebCore::ResourceError&) final;

I would have put these overrides inside the private section, since nobody needs to call them directly, only polymorphically through a ResourceHandleClient& to this object.
Comment 15 Chris Dumez 2016-09-24 19:08:19 PDT
Created attachment 289765 [details]
Patch
Comment 16 Chris Dumez 2016-09-24 19:09:20 PDT
*** Bug 156180 has been marked as a duplicate of this bug. ***
Comment 17 WebKit Commit Bot 2016-09-24 19:41:57 PDT
Comment on attachment 289765 [details]
Patch

Clearing flags on attachment: 289765

Committed r206356: <http://trac.webkit.org/changeset/206356>
Comment 18 WebKit Commit Bot 2016-09-24 19:42:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Alexey Proskuryakov 2016-09-25 17:37:54 PDT
This made tests crash on bots: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r206363%20(8387)/results.html

Chris, are you available to take a look?
Comment 20 Chris Dumez 2016-09-25 17:38:21 PDT
(In reply to comment #19)
> This made tests crash on bots:
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r206363%20(8387)/results.html
> 
> Chris, are you available to take a look?

Yes, looking.
Comment 21 Chris Dumez 2016-09-25 17:51:19 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > This made tests crash on bots:
> > https://build.webkit.org/results/
> > Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r206363%20(8387)/results.html
> > 
> > Chris, are you available to take a look?
> 
> Yes, looking.

I found the problem. I'll upload a patch shortly.
Comment 22 Chris Dumez 2016-09-25 19:07:32 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > This made tests crash on bots:
> > > https://build.webkit.org/results/
> > > Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r206363%20(8387)/results.html
> > > 
> > > Chris, are you available to take a look?
> > 
> > Yes, looking.
> 
> I found the problem. I'll upload a patch shortly.

Patch to address the crashes is up for review at https://bugs.webkit.org/show_bug.cgi?id=162541.