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

...

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

(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?

(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.

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.

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}]

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 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.

(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.

(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.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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

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

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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".

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.

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 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 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 on attachment 8476621 [details] [diff] [review] part 3 - whitespace and formatting cleanup of DoContent v.1 More cleanup in DoContent.

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.

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

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 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 on attachment 8476620 [details] [diff] [review] part 2 - move content process hacks out of DoContent v.1 r=me

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.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.

(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.

(In reply to Jim Mathies [:jimm] from comment #36) > (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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 on attachment 8476616 [details] [diff] [review] part 1 - use new interface in browser v.1 (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.

> 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.

(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 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 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?

(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. :)

- restored the original SchemeIs behavior.

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

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

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

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

> 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!

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

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

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

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.

(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).

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

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