1127927 - [e10s] Save Page As... doesn't work

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Dave Townsend [:mossop]]

Attempting to save page as in the current nightly seems to show the save start but the downloads panel just shows a full progress bar that seems stuck.

Comment 1

[Dave Townsend [:mossop]]

Bisecting shows this was caused by https://hg.mozilla.org/mozilla-central/rev/25ca03634cf5

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix-save-as

There were two problems here. The first is that we were passing a privacy context CPOW into saveURL. That's easy to fix.

The other problem is that we were passing this "cache key" in as a CPOW. This is similar to the problem we have with "View Source". The code didn't actually work with CPOWs, so I just changed it to pass null instead. I filed bug 1128050 to deal with the root problem.

Comment 3

[Jim Mathies [:jimm]]

Is bug 1128712 a dupe of this?

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> Is bug 1128712 a dupe of this?

I suspect so.  Both bugs have the same regression range for me (at least on OS X 10.7.5):

firefox-2015-01-29-03-02-02-mozilla-central
firefox-2015-01-30-03-02-32-mozilla-central

Note that this includes the patch from comment #1.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Same regression but different bug.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8557321 [details] [diff] [review]
fix-save-as

Review of attachment 8557321 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a privacy win too, with the switch to savePrivacyAwareURI. Good stuff!

Comment 8

[Tom S [:evilpie]]

Can we land this? I was just about to fix the same bug :)

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix-save-as v2

Sorry this is taking so long. I ended up folding this together with a patch for bug 1128712. I've tried to make it clearer in the code what is allowed to be a CPOW and what isn't. I also added a utility function, makeURIFromCPOW, that converts a URI around a CPOW into a process-local URI. I changed a couple places where we need this to use it. The advantage over makeURI is that it gets the charset right (not sure how important that is).

I'm still working on a test for this stuff. It's proving somewhat difficult to write.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8561655 [details] [diff] [review]
fix-save-as v2

Review of attachment 8561655 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +1195,5 @@
>            getService(Ci.nsIExternalHelperAppService);
>          let channel = aRequest.QueryInterface(Ci.nsIChannel);
>          this.extListener =
>            extHelperAppSvc.doContent(channel.contentType, aRequest, 
> +                                    null, true, window);

I'm guessing you'll have a question about this change, Mike. doContent takes two window arguments. The latter one is used to put up authorization prompts and stuff. We use the chrome process window for that already.

The former window (where we passed in doc.defaultView) is used in rare situations. Basically, if a link is for a file (or something that goes to the external helper app service), the server can stick an HTTP header on the response that causes the browser to display a certain web page when the file transfer is done ("Thanks for downloading!" or something like that). We use the window argument to set this up.

This code only runs when you choose "Save link" from the context menu. I'm pretty certain we don't want to do this refresh link business in that case.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8561655 [details] [diff] [review]
fix-save-as v2

Review of attachment 8561655 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +1195,5 @@
>            getService(Ci.nsIExternalHelperAppService);
>          let channel = aRequest.QueryInterface(Ci.nsIChannel);
>          this.extListener =
>            extHelperAppSvc.doContent(channel.contentType, aRequest, 
> +                                    null, true, window);

I'm guessing you'll have a question about this change, Mike. doContent takes two window arguments. The latter one is used to put up authorization prompts and stuff. We use the chrome process window for that already.

The former window (where we passed in doc.defaultView) is used in rare situations. Basically, if a link is for a file (or something that goes to the external helper app service), the server can stick an HTTP header on the response that causes the browser to display a certain web page when the file transfer is done ("Thanks for downloading!" or something like that). We use the window argument to set this up.

This code only runs when you choose "Save link" from the context menu. I'm pretty certain we don't want to do this refresh link business in that case.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix-save-as v3

Unfortunately, when I rebased this patch, the "Save link" functionality stopped working again. It was broken by changes that landed in bug 1087726. Christoph, could you please look over the changes I've made to newChannelFromURI2? In e10s, |doc| is actually a cross-process wrapper around a document in the content process. It's not safe to pass it into a chrome C++ method like this. So instead I passed the document principal. Is there any reason why you really need the node instead of just the principal?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8561791 [details] [diff] [review]
fix-save-as v3

Review of attachment 8561791 [details] [diff] [review]:
-----------------------------------------------------------------

Bill, ideally we store the |doc| as the loadingNode within the LoadInfo. Since serialization of a node does not work in e10s, we just query the loadingPrincipal from that loadingNode whenever the child opens a channel in the parent, see PropagateLoadinfo, called here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1597

If I am understanding correctly what you are saying, then we are facing a similar problem here, right?
I am fine with the change, but please answer my question underneath:

::: browser/base/content/nsContextMenu.js
@@ +1251,5 @@
>      var ioService = Cc["@mozilla.org/network/io-service;1"].
>                      getService(Ci.nsIIOService);
>      var channel = ioService.newChannelFromURI2(makeURI(linkURL),
> +                                               null, // aLoadingNode
> +                                               this.principal, // aLoadingPrincipal

I assume this.principal is the same as doc.nodePrincipal, right?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8561791 [details] [diff] [review]
fix-save-as v3

Review of attachment 8561791 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bill McCloskey (:billm) from comment #11)
> This code only runs when you choose "Save link" from the context menu. I'm
> pretty certain we don't want to do this refresh link business in that case.

Cool - thanks for the explanation. I agree that was don't want to do that for the context menu case. I'm actually kinda curious to know what happened before with the original code if the server sent such a header with this case.

::: browser/base/content/nsContextMenu.js
@@ +1251,5 @@
>      var ioService = Cc["@mozilla.org/network/io-service;1"].
>                      getService(Ci.nsIIOService);
>      var channel = ioService.newChannelFromURI2(makeURI(linkURL),
> +                                               null, // aLoadingNode
> +                                               this.principal, // aLoadingPrincipal

Yes, it should be the principal for the owner document that triggered the context menu event.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7e21a1fe735

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c7e21a1fe735

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Firefox/38.0

Reproduced the issue on 2015-02-12 Nightly.
Verified fixed on latest Nightly, build ID: 20150215030238