903022 - [e10s] Save content links as fails

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Comment 1

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

What is saveAs? Is there a place in the code where it's defined?

Comment 2

[Tom S [:evilpie]]

...

Comment 5

[Jim Mathies [:jimm]]

I'd suggest we move this to m1, it's pretty crippling for the user.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Jim Mathies [:jimm] from comment #5)
> I'd suggest we move this to m1, it's pretty crippling for the user.

I think the argument against m1 is that users don't save pages to disk very often. Do you disagree with that?

Comment 7

[Jim Mathies [:jimm]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6)
> (In reply to Jim Mathies [:jimm] from comment #5)
> > I'd suggest we move this to m1, it's pretty crippling for the user.
> 
> I think the argument against m1 is that users don't save pages to disk very
> often. Do you disagree with that?

They might not save pages but they save files. This bug applies to regular file links as well, like bugzilla attachments and file downloads.

Comment 8

[Ambrose Li]

I save pages to disk all the time (at least a few pages per day). Often I would forget that save doesn’t work on e10s until I try to quit Firefox, then I would have to copy each link from the download manager and reattempt the saves in Chrome.

Also, Save Page As and Save Link As are broken in different ways, at least in the two nightlies that I’ve tried (2014-07-03 and I believe 2014-06-25 or thereabouts). Save Page As results in a download that’s never finished, but Save Link As either does nothing or causes a crash.

Comment 9

[Jim Mathies [:jimm]]

This is what I see in the console - 

[JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Async version must be used" {file: "file:///F:/Mozilla/MC-REL/dist/bin/components/nsHelperAppDlg.js" line: 209}]

[JavaScript Error: "[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIFilePicker.init]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///F:/Mozilla/MC-REL/dist/bin/components/nsHelperAppDlg.js :: nsUnknownContentTypeDialog.prototype.promptForSaveToFileAsync/< :: line 259"  data: no]" {file: "resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js" line: 869}]

Comment 10

[Jim Mathies [:jimm]]

Attachment: prompt parent fix v.1

I think this is ok since I'm not replacing 'doc' so the cpow continues to provide information o the rest of this method. I'm just swapping out the parent window when instantiating the helper service.

todo: I need to check other handlers here to be sure the rest are working, and check the state of our existing tests related to the context menu.

Comment 11

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

Comment on attachment 8464963 [details] [diff] [review]
prompt parent fix v.1

This seems good to me, but Blake knows a lot more about the prompt stuff. I think this maybe makes the alert not be tab-modal, but I'm not sure. I kinda doubt we care though.

Comment 12

[Jim Mathies [:jimm]]

(In reply to Bill McCloskey (:billm) from comment #11)
> Comment on attachment 8464963 [details] [diff] [review]
> prompt parent fix v.1
> 
> This seems good to me, but Blake knows a lot more about the prompt stuff. I
> think this maybe makes the alert not be tab-modal, but I'm not sure. I kinda
> doubt we care though.

Interesting point, I just changed that prompt code while I was there. I'll do some testing to see what impact that change has.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to Bill McCloskey (:billm) from comment #11)
> This seems good to me, but Blake knows a lot more about the prompt stuff. I
> think this maybe makes the alert not be tab-modal, but I'm not sure. I kinda
> doubt we care though.

This particular prompt is not tab-modal. Passing the wrong window to doContent() seems potentially problematic, though.

Comment 15

[Jim Mathies [:jimm]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #14)
> (In reply to Bill McCloskey (:billm) from comment #11)
> > This seems good to me, but Blake knows a lot more about the prompt stuff. I
> > think this maybe makes the alert not be tab-modal, but I'm not sure. I kinda
> > doubt we care though.
> 
> This particular prompt is not tab-modal. Passing the wrong window to
> doContent() seems potentially problematic, though.

I took a look at this but didn't see any major issues. The underlying code does assume the window is the loader though, which makes this a bit of a sloppy fix.

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#614
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1154

Comment 16

[:Gavin Sharp [email: [e-mail]]]

This is hacky enough to merit a FIXME comment+followup bug or #ifdef E10S_TESTING_ONLY.

Comment 17

[Jim Mathies [:jimm]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #16)
> This is hacky enough to merit a FIXME comment+followup bug or #ifdef
> E10S_TESTING_ONLY.

Well, I'd rather find a good fix if possible that doesn't require a follow up.

What I would like to do here is wrap the window cpow in some sort of local object that supports the right interfaces. But I can't handle the c++ nsPIDOMWindow interface this way for this call -

http://mxr.mozilla.org/mozilla-central/source/widget/shared/SharedWidgetUtils.cpp#21 

Also generally we don't want cpows down in c++, which is an additional wrinkle here. billm has been thinking about blocking use of cpows in C++ since they basically shouldn't ever get down to that level since if they do they fail in unexpected ways like in this bug.

ni'ing bill, maybe he has some ideas on how to go about this the "right way".

Comment 18

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

I looked at this some more an I don't actually see any problems with passing in the chrome window to doContent. I also talked to Gavin about it. His concern is that we're using the window parameter to do something important. However, I don't *think* that's happening. Ultimately the window gets stored in mWindowContext of the nsExternalAppHelper. The only place where I see we use that window for stuff besides prompting and UI is in RetargetLoadNotifications.

However, I don't think that code is doing anything in this case. It all seems to relate to the comment here:
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2445
In the case of "Save link as...", we definitely don't want to do anything with this refresh interface. So I don't think there's any need to compute the original, pre-redirect URL.

When I talked to Gavin, he said he didn't have any problems with this change as it doesn't break anything and as long as we document when and why it's safe. Somebody who understands the networking code really needs to confirm what I've said though.

Comment 21

[Jim Mathies [:jimm]]

Attachment: wip

I didn't feel comfortable dropping the browser window down into nsExternalHelperAppService so I've added a new interface / api that supports what we need here, and made the necessary changes to nsExternalHelperAppService to support it. Pushing this to try currently, if everything comes up green I'll kick off some reviews on a broken down patch set.

https://tbpl.mozilla.org/?tree=Try&rev=daf7033294c3

Comment 22

[Jim Mathies [:jimm]]

Attachment: part 1 - use new interface in browser v.1

Comment 23

[Jim Mathies [:jimm]]

Attachment: part 2 - move content process hacks out of DoContent v.1

Comment 24

[Jim Mathies [:jimm]]

Attachment: part 3 - whitespace and formatting cleanup of DoContent v.1

Comment 25

[Jim Mathies [:jimm]]

Attachment: part 4 - introduce new nsIExternalHelperAppService2 interface v.1

Comment 26

[Jim Mathies [:jimm]]

Attachment: part 5 - update nsExternalAppHandler to support new dialog parenting v.1

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 8476620 [details] [diff] [review]
part 2 - move content process hacks out of DoContent v.1

This actually landed back when we were working on e10s for fennec. Follow up bug 564315 was filed to fix this the right way. That's currently tagged as e10s-later.

Comment 28

[Jim Mathies [:jimm]]

Comment on attachment 8476620 [details] [diff] [review]
part 2 - move content process hacks out of DoContent v.1

This patch is purely code relocation with some formatting cleanup.

Comment 29

[Jim Mathies [:jimm]]

Comment on attachment 8476621 [details] [diff] [review]
part 3 - whitespace and formatting cleanup of DoContent v.1

More cleanup in DoContent.

Comment 30

[Jim Mathies [:jimm]]

Comment on attachment 8476623 [details] [diff] [review]
part 4 - introduce new nsIExternalHelperAppService2 interface v.1

This adds an extension to nsIExternalHelperAppService to provide a new DoContent method that takes both a dialog parent(browser window) and the original content doc for things like page refresh after download.

Comment 31

[Jim Mathies [:jimm]]

try push:
https://tbpl.mozilla.org/?tree=Try&rev=687c7fa7112b

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8476620 [details] [diff] [review]
part 2 - move content process hacks out of DoContent v.1

This sounds good to me, but bz should review these changes.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8476616 [details] [diff] [review]
part 1 - use new interface in browser v.1

I guess I have two questions:
- do we really need nsIExternalHelperAppService2? Can we just change nsIExternalHelperAppService instead?
- Why not just always take the "isRemote" path here?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8476620 [details] [diff] [review]
part 2 - move content process hacks out of DoContent v.1

r=me

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8476621 [details] [diff] [review]
part 3 - whitespace and formatting cleanup of DoContent v.1

>+        bool isHTTP = false, isHTTPS = false;

This is not equivalent to the old code.  Why is this change needed?

The rest of this looks fine.

Comment 36

[Jim Mathies [:jimm]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #33)
> Comment on attachment 8476616 [details] [diff] [review]
> part 1 - use new interface in browser v.1
> 
> I guess I have two questions:
> - do we really need nsIExternalHelperAppService2? Can we just change
> nsIExternalHelperAppService instead?

It's been around for ages, and DoContent is the main method people use. I was worried about breaking the world in modifying the original DoContent. I considered just adding a new method to the original interface, but I think that would break binary extensions?

> - Why not just always take the "isRemote" path here?

I don't understand what you mean by the "isRemote" path.

Comment 37

[Jim Mathies [:jimm]]

(In reply to Boriz Zbarsky [:bz] from comment #35)
> Comment on attachment 8476621 [details] [diff] [review]
> part 3 - whitespace and formatting cleanup of DoContent v.1
> 
> >+        bool isHTTP = false, isHTTPS = false;
> 
> This is not equivalent to the old code.  Why is this change needed?

This looks equivalent to me -

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsSimpleURI.cpp#460

o_Equals doesn't get set unless the call succeeds.

Comment 38

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #36)
> (In reply to :Gavin Sharp [email: [e-mail]] from comment #33)
> > Comment on attachment 8476616 [details] [diff] [review]
> > part 1 - use new interface in browser v.1
> > 
> > I guess I have two questions:
> > - do we really need nsIExternalHelperAppService2? Can we just change
> > nsIExternalHelperAppService instead?
> 
> It's been around for ages, and DoContent is the main method people use. I
> was worried about breaking the world in modifying the original DoContent. I
> considered just adding a new method to the original interface, but I think
> that would break binary extensions?
> 
> > - Why not just always take the "isRemote" path here?
> 
> I don't understand what you mean by the "isRemote" path.

sorry! you were looking at a different patch.

Comment 39

[Jim Mathies [:jimm]]

Comment on attachment 8476616 [details] [diff] [review]
part 1 - use new interface in browser v.1

(In reply to :Gavin Sharp [email: [e-mail]] from comment #33)
> - Why not just always take the "isRemote" path here?

Yes, looks like I can just call the new method regardless. You jumped ahead on me. :) Aside from that, does this look ok here? Note, I didn't change the prompt window above this. I think prompts should work with the cpow'd window.

Comment 41

[Boris Zbarsky [:bzbarsky]]

> This looks equivalent to me -

For nsSimpleURI, yes.  But you don't know you have an nsSimpleURI.  You could have some extension-provided nsIURI implementation.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jim Mathies [:jimm] from comment #36)
> It's been around for ages, and DoContent is the main method people use. I
> was worried about breaking the world in modifying the original DoContent. I
> considered just adding a new method to the original interface, but I think
> that would break binary extensions?

Breaking binary extensions on trunk is fine. Having to update a million callers might not be, but is that really the case here?

Comment 43

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8476616 [details] [diff] [review]
part 1 - use new interface in browser v.1

This looks fine to me if you remove the isRemote check and just always use "window" as the dialogParent. r=me with that change, and pending a decision on whether we really need nsIExternalHelperAppService2.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8476623 [details] [diff] [review]
part 4 - introduce new nsIExternalHelperAppService2 interface v.1

A few things:

1)  Even if we add a new interface, that does not require a new contractid.  A contract means some particular set of interfaces is all.

2)  There is no problem "breaking" binary addons in the sense of making them recompile.  As long as you rev the iid when you modify an interface, it's fine.

3)  You could just add an optional argument to the existing doContent, no?  That would keep compat with JS callers.  C++ ones would need to trivially edit to pass nullptr and recompile, but that's ok.

4)  aContentContext is unused as far as I can tell.  That seems wrong?

Comment 45

[Jim Mathies [:jimm]]

(In reply to Boriz Zbarsky [:bz] from comment #44)
> Comment on attachment 8476623 [details] [diff] [review]
> part 4 - introduce new nsIExternalHelperAppService2 interface v.1
> 
> A few things:
> 
> 1)  Even if we add a new interface, that does not require a new contractid. 
> A contract means some particular set of interfaces is all.
> 
> 2)  There is no problem "breaking" binary addons in the sense of making them
> recompile.  As long as you rev the iid when you modify an interface, it's
> fine.
> 
> 3)  You could just add an optional argument to the existing doContent, no? 
> That would keep compat with JS callers.  C++ ones would need to trivially
> edit to pass nullptr and recompile, but that's ok.

Ok sounds good, I'll go with #3 here and add a new window context pointer on the end of DoContent, rev the iid on the original interface, and update any c++ callers on mc and cc.

> 4)  aContentContext is unused as far as I can tell.  That seems wrong?

oops, that was a bug in DoContent2, DoContent should have received aContentContext. No longer relevant, nice catch though. :)

Comment 46

[Jim Mathies [:jimm]]

Attachment: part 3 - whitespace and formatting cleanup of DoContent v.2 (r=bzbarsky)

- restored the original SchemeIs behavior.

Comment 47

[Jim Mathies [:jimm]]

Attachment: part 1 - use new interface in browser v.2 (r=gavin)

- revert to the original (modified) doContent method
- always pass window

Comment 48

[Jim Mathies [:jimm]]

Attachment: part 4 - add support for new dialog parenting v.2

Comment 49

[Jim Mathies [:jimm]]

Attachment: part 4 - add support for new dialog parenting v.2

Combined changes to the handler service and handler app. I also did a little formatting cleanup in the areas I touched.

Comment 50

[Jim Mathies [:jimm]]

try push:

https://tbpl.mozilla.org/?tree=Try&rev=23ebe620ea9b

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8477362 [details] [diff] [review]
part 4 - add support for new dialog parenting v.2

Should the interface requestor argument to DoContentContentProcessHelper be called aContentContext?

And why not pass in both contexts to it?

>+            if(NS_SUCCEEDED(bundle->FormatStringFromName(msgId.get(), strings, 1, getter_Copies(msgText)))) {

If you're touching this anyway (which would be much better done in a patch that doesn't also make substantive changes), at least add a space after the "if"?

But really, it would be best for cleanup to not be in the same changeset as actual changes.  :(

r=me

Comment 52

[Jim Mathies [:jimm]]

> Should the interface requestor argument to DoContentContentProcessHelper be
> called aContentContext?
>
> And why not pass in both contexts to it?

Good suggestions, will update.

> But really, it would be best for cleanup to not be in the same changeset as
> actual changes.  :(

I'll split the whitespace stuff out into a separate cset.

Thanks for the timely reviews!

Comment 53

[Jim Mathies [:jimm]]

Attachment: split out white space changes

Comment 54

[Jim Mathies [:jimm]]

final try build - 
https://tbpl.mozilla.org/?tree=Try&rev=120747cba5f9

Comment 55

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22bf1f1b1c50
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55cf90070847
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b62223a2c577
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc2c5dbf796
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b793b16ee15c

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/22bf1f1b1c50
https://hg.mozilla.org/mozilla-central/rev/55cf90070847
https://hg.mozilla.org/mozilla-central/rev/b62223a2c577
https://hg.mozilla.org/mozilla-central/rev/bdc2c5dbf796
https://hg.mozilla.org/mozilla-central/rev/b793b16ee15c

Comment 57

[(no longer active)]

This is not fixed as of the latest nightly from https://hg.mozilla.org/mozilla-central/rev/dc352a7bf234.  Hitting File -> Save Page as doesn't do anything for me.

Comment 58

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #57)
> This is not fixed as of the latest nightly from
> https://hg.mozilla.org/mozilla-central/rev/dc352a7bf234.  Hitting File ->
> Save Page as doesn't do anything for me.

What OS?  I filed bug 1058251 for my observation that it doesn't work on Linux (and, I later found, OS X).

Comment 59

[Jim Mathies [:jimm]]

Apparently save page as is a completely different issue, and is covered by bug 1058251.

Comment 60

[[:Aleksej]]

Verified fixed:
bug: 2014-08-24-03-02-10-mozilla-central-firefox-34.0a1.ru.linux-x86_64
WFM: 2014-08-27-03-02-02-mozilla-central-firefox-34.0a1.ru.linux-x86_64