[e10s] link target no longer saved when browser.altClickSave is set to True

RESOLVED FIXED in Firefox 51

Status

()

Firefox
File Handling
P1
normal
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: rgoubet, Assigned: jimicy)

Tracking

34 Branch
Firefox 51
Points:
---

Firefox Tracking Flags

(e10s+, firefox51 fixed)

Details

(Whiteboard: [fce-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141125180439

Steps to reproduce:

Alt-click on a link when browser.altClickSave is True


Actual results:

Target opened in a new tab (same as ctrl-click)


Expected results:

Target should be saved

Comment 1

3 years ago
bugday-20141216 :Expected result does not occur.
Flags: needinfo?(pratyasmitamishra)

Updated

2 years ago
Component: Untriaged → File Handling

Comment 2

2 years ago
I can't reproduce this issue on Firefox 35 (upcoming release, currently in beta: https://beta.mozilla.org ) using the "show info" link above (next to the "Product" dropdown). Can you provide an example page where you see this, and test on a clean profile ( https://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-profiles ) - with the pref set? I expect an add-on is interfering with this functionality...
Flags: needinfo?(rgoubet)

Comment 3

2 years ago
I can reproduce this on 41.0a2 Linux GTK3. While e10s is enabled, browser.altClickSave does not work.

Comment 4

2 years ago
(In reply to Jan Steffens from comment #3)
> I can reproduce this on 41.0a2 Linux GTK3. While e10s is enabled,
> browser.altClickSave does not work.

But not when e10s is not enabled?
Flags: needinfo?(rgoubet)
Flags: needinfo?(pratyasmitamishra)
Flags: needinfo?(jan.steffens)

Comment 5

2 years ago
Right. When e10s is disabled (either globally or in a non-e10s window) links are saved by alt+click as expected.
Flags: needinfo?(jan.steffens)

Updated

2 years ago
Status: UNCONFIRMED → NEW
tracking-e10s: --- → ?
Ever confirmed: true
OS: Windows 8.1 → All
Hardware: x86_64 → All
Summary: Link target no longer saved when browser.altClickSave is set to True → [e10s] link target no longer saved when browser.altClickSave is set to True
Assignee: nobody → felipc
tracking-e10s: ? → m8+

Comment 6

2 years ago
Browser Console shows:

12:06:24.736 openUILink/openLinkIn was called with where == 'save' but without initiatingDoc.  See bug 814264.1 utilityOverlay.js:217:0
Created attachment 8661057 [details]
MozReview Request: Bug 1109146 - Move whereToOpenLink to BrowserUtils to be used by parent and child processes. r?mak

Bug 1109146 - Move whereToOpenLink to BrowserUtils to be used by parent and child processes. r?mak
Attachment #8661057 - Flags: review?(mak77)
Created attachment 8661058 [details]
MozReview Request: Bug 1109146 - Move gatherTextUnder to BrowserUtils to be used by parent and child processes. r?mak

Bug 1109146 - Move gatherTextUnder to BrowserUtils to be used by parent and child processes. r?mak
Attachment #8661058 - Flags: review?(mak77)
Created attachment 8661059 [details]
MozReview Request: Bug 1109146 - Support click target "save" for remote content. r?mak

Bug 1109146 - Support click target "save" for remote content. r?mak
Attachment #8661059 - Flags: review?(mak77)
Created attachment 8661061 [details] [diff] [review]
Alternative patch

So, there's already support in saveURL to be called with a initiatingDoc. That is only used to determine PB-ness, and saveURL allows aPrivate to be passed in, in case initiatingDoc is not given. I have an alternative implementation that uses this and is a bit less intrusive, so I'm posting it here for reference.

The problem with this alternative is that it doesn't support naming the file with the gatherTextUnder function, because the whereToOpenLink function is still called in parent, and then we would need to call gatherTextUnder for the node in content. We can't call it on every mouse click (and let the parent decide to use it or not), because that would be very expensive. So on the implementation that I posted for review, whereToOpenLink and gatherTextUnder are moved to content.
Comment on attachment 8661061 [details] [diff] [review]
Alternative patch

For simplicity and to avoid confusion, I'm gonna mark the alternative patch as obsolete, as I think it's not worth spending time to look at it since it's an incomplete solution
Attachment #8661061 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/19257/#review18051

::: toolkit/modules/BrowserUtils.jsm:24
(Diff revision 1)
> +}

Why not use Preferences.jsm?

::: toolkit/modules/BrowserUtils.jsm:441
(Diff revision 1)
> +  /* whereToOpenLink() looks at an event to decide where to open a link.

please make this a proper javadoc, as
/**
 * ...

also, usually we don't repeat the function name.
https://reviewboard.mozilla.org/r/19257/#review18053

::: toolkit/modules/BrowserUtils.jsm:491
(Diff revision 1)
> +  /*

ditto for making this a proper javadoc
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2cc105233ff
Felipe, this is still reported as pending review for whatever reason, there are 3 issues still open in RB... Let me know if I should do anything further now.
Flags: needinfo?(felipc)
Attachment #8661057 - Attachment is obsolete: true
Attachment #8661057 - Flags: review?(mak77)
Attachment #8661058 - Attachment is obsolete: true
Attachment #8661058 - Flags: review?(mak77)
Attachment #8661059 - Attachment is obsolete: true
Attachment #8661059 - Flags: review?(mak77)
Sorry Mak, it was just some RB weirdness. I'm fixing the test failure and will upload a new version with your comments addressed too
Flags: needinfo?(felipc)
Renoming.. I think this would be nice to have but definitely not a beta blocker.. It's a prob that only happens for a non-default pref value.
tracking-e10s: m8+ → ?
tracking-e10s: ? → +
Assignee: felipc → nobody

Comment 18

a year ago
Error in Browser console:

openUILink/openLinkIn was called with where == 'save' but without initiatingDoc.  See bug 814264. utilityOverlay.js:227:0
I can reproduce this issue on Beta 45.0b3 when e10s is enabled.

I am unable to reproduce this issue on Beta 45.0b3 when e10s is disabled.
Priority: -- → P1
Created attachment 8750813 [details]
Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;

Review commit: https://reviewboard.mozilla.org/r/51651/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51651/
Assignee: nobody → jimmyw22
Created attachment 8750926 [details]
Bug 1109146 - skip prompt for downloading a file when you alt+click a link;

Review commit: https://reviewboard.mozilla.org/r/51707/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51707/
Comment on attachment 8750813 [details]
Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51651/diff/1-2/
Comment on attachment 8750813 [details]
Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51651/diff/2-3/
Attachment #8750813 - Attachment description: MozReview Request: Bug 1109146 - Pass initiating document from context click to utilityOverlay.js. → MozReview Request: Bug 1109146 - Pass the initiating document from content click (alt+click on a link) to utilityOverlay.js to be used in saveURL to save the appropriate file; r?felipe
Attachment #8750926 - Attachment description: MozReview Request: Bug 1109146 - change to skip prompt for downloading a file. → MozReview Request: Bug 1109146 - skip prompt for downloading a file when you alt+click a link; r?felipe
Attachment #8750813 - Flags: review?(felipc)
Attachment #8750926 - Flags: review?(felipc)
Comment on attachment 8750926 [details]
Bug 1109146 - skip prompt for downloading a file when you alt+click a link;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51707/diff/1-2/
Hi Felipe, do you know what this comment in ContentClick.jsm means?
> // Todo(903022): code for where == save

Bug 903022 seems to be resolved and it isn't related to where == save
Flags: needinfo?(felipc)
Created attachment 8754073 [details]
Bug 1109146 - write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick();

Review commit: https://reviewboard.mozilla.org/r/53708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53708/
Attachment #8750813 - Attachment description: MozReview Request: Bug 1109146 - Pass the initiating document from content click (alt+click on a link) to utilityOverlay.js to be used in saveURL to save the appropriate file; r?felipe → MozReview Request: Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;r?felipe
Comment on attachment 8750926 [details]
Bug 1109146 - skip prompt for downloading a file when you alt+click a link;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51707/diff/2-3/
Comment on attachment 8750813 [details]
Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51651/diff/3-4/
Comment on attachment 8754073 [details]
Bug 1109146 - write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick();

I'm writing a test for alt+click to download a file. My tests attempts to download a .htm file from a link. This file doesn't exist so I get a failed download.

I just check that list._downloads[0].source.url is the correct link for the failed-to-download file as my test.

However, I'm getting an error that is causing my test to fail.
NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.responseStatus]
DownloadLegacy.js:96

I noticed this error also happens in firefox when you get a failed download. Because in DownloadLegacy.js:96, aRequest.responseStatus doesn't exist for a failed download

Should we be error-ing out like this? An example html file https://www.dropbox.com/s/s6m1bd34tk0v651/test.html?dl=0. None of the links exist so they all result in a failed download.
Flags: needinfo?(paolo.mozmail)

Comment 30

a year ago
(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #29)
> However, I'm getting an error that is causing my test to fail.
> NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111
> (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.responseStatus]
> DownloadLegacy.js:96
> 
> I noticed this error also happens in firefox when you get a failed download.
> Because in DownloadLegacy.js:96, aRequest.responseStatus doesn't exist for a
> failed download
> 
> Should we be error-ing out like this? An example html file
> https://www.dropbox.com/s/s6m1bd34tk0v651/test.html?dl=0. None of the links
> exist so they all result in a failed download.

Sorry for the delayed response - sounds like a bug in the Downloads.jsm code. My first thought is that we should wrap the responseStatus call in a try-catch block. Can you file a bug for this and see if the try-catch solves your issue? We should add a test to "common_test_Download.js" for this case.
Flags: needinfo?(paolo.mozmail)
Depends on: 1275289
Comment on attachment 8750926 [details]
Bug 1109146 - skip prompt for downloading a file when you alt+click a link;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51707/diff/3-4/
Comment on attachment 8750813 [details]
Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51651/diff/4-5/
Comment on attachment 8754073 [details]
Bug 1109146 - write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53708/diff/1-2/
Attachment #8750926 - Flags: review?(felipc) → review+
Comment on attachment 8750926 [details]
Bug 1109146 - skip prompt for downloading a file when you alt+click a link;

https://reviewboard.mozilla.org/r/51707/#review52996
Attachment #8750813 - Flags: review?(felipc) → review+
Comment on attachment 8750813 [details]
Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;

https://reviewboard.mozilla.org/r/51651/#review52998
Comment on attachment 8754073 [details]
Bug 1109146 - write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick();

https://reviewboard.mozilla.org/r/53708/#review53000
Attachment #8754073 - Flags: review+
Hmm I think that comment can be removed but feel free to leave that in for landing, and let's create a new bug to dig in the bug history
Flags: needinfo?(felipc)
Whiteboard: [fce-active]
Comment on attachment 8750926 [details]
Bug 1109146 - skip prompt for downloading a file when you alt+click a link;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51707/diff/4-5/
Attachment #8750926 - Attachment description: MozReview Request: Bug 1109146 - skip prompt for downloading a file when you alt+click a link; r?felipe → Bug 1109146 - skip prompt for downloading a file when you alt+click a link;
Attachment #8750813 - Attachment description: MozReview Request: Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;r?felipe → Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;
Attachment #8754073 - Attachment description: MozReview Request: Bug 1109146 - write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick(); → Bug 1109146 - write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick();
Attachment #8754073 - Flags: review+
Comment on attachment 8750813 [details]
Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51651/diff/5-6/
Comment on attachment 8754073 [details]
Bug 1109146 - write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53708/diff/2-3/
Comment on attachment 8750926 [details]
Bug 1109146 - skip prompt for downloading a file when you alt+click a link;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51707/diff/5-6/
Comment on attachment 8750813 [details]
Bug 1109146 - pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51651/diff/6-7/
Comment on attachment 8754073 [details]
Bug 1109146 - write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53708/diff/3-4/
Please check in Bug 1275289 first.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0008e250eb38&selectedJob=24794560
Keywords: checkin-needed

Comment 45

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0212e01c7769
Skip prompt for downloading a file when you alt+click a link. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/65d2a173cb20
Pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/77ed89a0e8ba
Write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick(). r=felipe
Keywords: checkin-needed

Comment 46

10 months ago
All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7d72948689 for being the likely cause of m(bc) failures like https://treeherder.mozilla.org/logviewer.html#?job_id=32934749&repo=mozilla-inbound
Flags: needinfo?(jimmyw22)

Comment 47

9 months ago
(In reply to Wes Kocher (:KWierso) from comment #46)
> All backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7d72948689 for
> being the likely cause of m(bc) failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=32934749&repo=mozilla-
> inbound

Hi, Jimmy,

Any update regarding this comment?
Are we going to fix failures soon or later?
(In reply to William Hsu [:whsu] from comment #47)
> (In reply to Wes Kocher (:KWierso) from comment #46)
> > All backed out in
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7d72948689 for
> > being the likely cause of m(bc) failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=32934749&repo=mozilla-
> > inbound
> 
> Hi, Jimmy,
> 
> Any update regarding this comment?
> Are we going to fix failures soon or later?

Working on it right now. Thanks for checking up William.
Flags: needinfo?(jimmyw22)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Fixed failing test.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d527e9fa5c7&selectedJob=26067870

Check in Bug 1275289 first.
Keywords: checkin-needed

Comment 53

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1915e2586177
skip prompt for downloading a file when you alt+click a link; r=Felipe
https://hg.mozilla.org/integration/autoland/rev/86872f510751
pass isContentWindowPrivate from ContentClick.jsm to utilityOverlay.js for saveURL instead of passing a CPOW initiating content document;r=Felipe
https://hg.mozilla.org/integration/autoland/rev/8e2c02a74e75
write test to verify alt+click behaviour in e10s since the codepath is ContentClick.jsm instead of browser.js::contentAreaClick(); r=Felipe
Keywords: checkin-needed

Comment 54

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1915e2586177
https://hg.mozilla.org/mozilla-central/rev/86872f510751
https://hg.mozilla.org/mozilla-central/rev/8e2c02a74e75
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have reproduced this on firefox nightly according to(2014-12-09)

Fixing bug is verified on Latest Nightly-- Build ID:(20160908030434 ), User Agent: Mozilla/5.0 (Windows NT 10.0; rv:51.0) Gecko/20100101 Firefox/51.0


Tested OS--Windows10 32bit
QA Whiteboard: [testday-20160909]

Comment 56

4 months ago
[bugday-20170118] Bug verified
You need to log in before you can comment on or make changes to this bug.