1589270 - Handle Content-Disposition:attachment downloads in the parent process with DocumentChannel

Status: RESOLVED FIXED

(Core :: Networking, task, P2)

Description

[Matt Woodrow (:mattwoodrow)]

This is the first piece of bug 1574372. We trigger downloading when explicitly requested (this bug), and also when all the other potential content handlers can't handle the mime type.

This converts the download handler to be able to run directly in the parent, even when the docshell is in a content process.

The rest of bug 1574372 will add support for querying the content handlers for supported types from the parent, such that we can determine if we need to take the fallback download path.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1589270 - Part 1: Allow passing a specific status when disconnecting the child side, but only pass it to nsDocumentOpenInfo, not the load group. r?bzbarsky

We don't want to fire OnStartRequest to nsDocumentOpenInfo, since that will also trigger content handling and downloads. Just removing from the load group is sufficient, and results in OnStopRequest being sent to the doc shell.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1589270 - Part 2: Remove support for handling the Refresh header while processing a download.

The existing comment here doesn't make much sense, since the doc loader's document channel gets updated on redirects.
I also don't see why we would want the pre-redirect channel. If we get redirected to a download, we should be using the Refresh header from the request that gave us the download, not something earlier that redirected.

This also wouldn't work in the parent processi (since there's no docshell or loader there), which is the only place the Refresh header is ever processed.

Depends on D49526

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1589270 - Part 3: Convert nsExternalHelperApp to use BrowsingContext instead of nsIInterfaceRequestor.

This also converts MaybeCloseWindowHelper, and results in the window close operations being always run in the parent (even without DocumentChannel).

Depends on D49527

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1589270 - Part 4: Add support for processing the Refresh header on a download (using BrowsingContext), when the docshell is in a different process.

I believe this was broken with the e10s rewrite.

Depends on D49528

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1589270 - Part 5: Add support for handling Content-Disposition: attachment downloads directly in the parent process using DocumentChannelParent

Depends on D49529

Comment 6

[Matt Woodrow (:mattwoodrow)]

Some extra information:

The current e10s solution for handling this code is that we run nsExternalAppHandler in both the content process and the parent process. We use PExternalHelperApp to forward OnStart/StopRequest etc from the content process instance to the parent process instance.

Some of the actual code only runs in one process or the other though, depending on which QIs succeed.

mOriginalChannel is only set in the content process, since it tries to get the DocLoader.
ProcessAnyRefreshTags only runs in the parent process (called within the actual saving code), but needs mOriginalChannel. I think this is just busted with e10s.
Close window handling only runs in the content process, because we QI to a nsIPropertyBag2 before requesting it, which will only work if we have access to the docshell.
The actual download and save core code only runs in the parent process, due to an explicit XRE_IsContentProcess() check.

The plan here is to eventually only support multi-process downloads via DocumentChannel, and have a single instance of nsExternalAppHandler (and delete PExternalHelperApp).

These patches simplify the two-instance case a bit (like moving window closing to always run in the parent and fixing the refresh header handling, using BrowsingContext), but don't try too hard to clean up the to-be-deleted code.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1589270 - Part 6: Add test for handling the HTTP refresh header when we trigger a download. r?bzbarsky

Depends on D49530

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73f846dfc5b1
Part 1: Allow passing a specific status when disconnecting the child side, but only pass it to nsDocumentOpenInfo, not the load group. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/68bc2ae9b89a
Part 2: Remove support for handling the Refresh header while processing a download. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/baffa8a7b961
Part 3: Convert nsExternalHelperApp to use BrowsingContext instead of nsIInterfaceRequestor. r=bzbarsky

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/73f846dfc5b1
https://hg.mozilla.org/mozilla-central/rev/68bc2ae9b89a
https://hg.mozilla.org/mozilla-central/rev/baffa8a7b961

Comment 10

[Kohei Yoshino [:kohei]]

(I’m not going to write a site compatibility note because, as per Matt, it has already been broken since e10s was introduced.)

Comment 11

[Chris Peterson [:cpeterson]]

Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732