[e10s] Save content links as fails

VERIFIED FIXED in Firefox 34

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: evilpie, Assigned: jimm)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 34
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox34 verified)

Details

Attachments

(5 attachments, 7 obsolete attachments)

8.64 KB, patch
Details | Diff | Splinter Review
5.67 KB, patch
Details | Diff | Splinter Review
1.46 KB, patch
Details | Diff | Splinter Review
23.92 KB, patch
Details | Diff | Splinter Review
18.92 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
What is saveAs? Is there a place in the code where it's defined?
(Reporter)

Comment 2

5 years ago
...
Summary: Implement saveAs for e10s → Implement saveURL for e10s
tracking-e10s: --- → +
Blocks: 997462
Duplicate of this bug: 960928
Duplicate of this bug: 1005996
Summary: Implement saveURL for e10s → [e10s] Save pages to disk (Save link as, Save page as, etc.)
(Assignee)

Comment 5

4 years ago
I'd suggest we move this to m1, it's pretty crippling for the user.
Flags: needinfo?(blassey.bugs)
(Assignee)

Updated

4 years ago
OS: Linux → All
(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?
Flags: needinfo?(blassey.bugs) → needinfo?(jmathies)
(Assignee)

Comment 7

4 years ago
(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.
Flags: needinfo?(jmathies)

Comment 8

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → jmathies
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

4 years ago
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}]
Priority: -- → P1
(Assignee)

Updated

4 years ago
No longer depends on: 903016
(Assignee)

Comment 10

4 years ago
Created attachment 8464963 [details] [diff] [review]
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.
Attachment #8464963 - Flags: feedback?(wmccloskey)
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.
Attachment #8464963 - Flags: feedback?(wmccloskey) → feedback?(mrbkap)
(Assignee)

Comment 12

4 years ago
(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.
(Assignee)

Comment 13

4 years ago
(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.
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
(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".
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

4 years ago
Attachment #8464963 - Flags: feedback?(mrbkap)
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.
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1047794
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1047793
(Assignee)

Updated

4 years ago
Attachment #8464963 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 8476138 [details] [diff] [review]
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
(Assignee)

Comment 22

4 years ago
Created attachment 8476616 [details] [diff] [review]
part 1 - use new interface in browser v.1
Attachment #8476138 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
Created attachment 8476620 [details] [diff] [review]
part 2 - move content process hacks out of DoContent v.1
(Assignee)

Comment 24

4 years ago
Created attachment 8476621 [details] [diff] [review]
part 3 - whitespace and formatting cleanup of DoContent v.1
(Assignee)

Comment 25

4 years ago
Created attachment 8476623 [details] [diff] [review]
part 4 - introduce new nsIExternalHelperAppService2 interface v.1
(Assignee)

Comment 26

4 years ago
Created attachment 8476624 [details] [diff] [review]
part 5 - update nsExternalAppHandler to support new dialog parenting v.1
(Assignee)

Comment 27

4 years ago
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.
Attachment #8476620 - Attachment description: part 2 - move b2g hacks out of DoContent v.1 → part 2 - move content process hacks out of DoContent v.1
(Assignee)

Comment 28

4 years ago
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.
Attachment #8476620 - Flags: review?(gavin.sharp)
(Assignee)

Comment 29

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

More cleanup in DoContent.
Attachment #8476621 - Flags: review?(gavin.sharp)
(Assignee)

Comment 30

4 years ago
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.
Attachment #8476623 - Flags: review?(bzbarsky)
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.
Attachment #8476620 - Flags: review?(gavin.sharp) → review?(bzbarsky)
Attachment #8476621 - Flags: review?(gavin.sharp) → review?(bzbarsky)
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
Attachment #8476620 - Flags: review?(bzbarsky) → review+
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.
Attachment #8476621 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 36

4 years ago
(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.
(Assignee)

Comment 37

4 years ago
(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.
(Assignee)

Comment 38

4 years ago
(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.
(Assignee)

Comment 39

4 years ago
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.
Attachment #8476616 - Flags: review?(gavin.sharp)
(Assignee)

Comment 40

4 years ago
> 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.
Attachment #8476616 - Flags: review?(gavin.sharp) → review+
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?
Attachment #8476623 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 45

4 years ago
(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. :)
(Assignee)

Comment 46

4 years ago
Created attachment 8476855 [details] [diff] [review]
part 3 - whitespace and formatting cleanup of DoContent v.2 (r=bzbarsky)

- restored the original SchemeIs behavior.
Attachment #8476621 - Attachment is obsolete: true
(Assignee)

Comment 47

4 years ago
Created attachment 8476878 [details] [diff] [review]
part 1 - use new interface in browser v.2 (r=gavin)

- revert to the original (modified) doContent method
- always pass window
Attachment #8476616 - Attachment is obsolete: true
(Assignee)

Comment 48

4 years ago
Created attachment 8477357 [details] [diff] [review]
part 4 - add support for new dialog parenting v.2
Attachment #8476623 - Attachment is obsolete: true
Attachment #8476624 - Attachment is obsolete: true
(Assignee)

Comment 49

4 years ago
Created attachment 8477362 [details] [diff] [review]
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.
Attachment #8477357 - Attachment is obsolete: true
Attachment #8477362 - Flags: review?(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
Attachment #8477362 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 52

4 years ago
> 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!
(Assignee)

Comment 53

4 years ago
Created attachment 8478298 [details] [diff] [review]
split out white space changes
Blocks: 1058251

Comment 57

4 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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).
(Assignee)

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 59

4 years ago
Apparently save page as is a completely different issue, and is covered by bug 1058251.
Summary: [e10s] Save pages to disk (Save link as, Save page as, etc.) → [e10s] Save content links as fails

Comment 60

4 years ago
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
QA Whiteboard: [bugday-20140827]
Status: RESOLVED → VERIFIED
status-firefox34: --- → verified
Blocks: 1101100
You need to log in before you can comment on or make changes to this bug.