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

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P1)

Description

[rgoubet]

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

[pratyasmitamishra]

bugday-20141216 :Expected result does not occur.

Comment 2

[:Gijs (he/him)]

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

Comment 3

[Jan Alexander Steffens [:heftig]]

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

Comment 4

[:Gijs (he/him)]

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

Comment 5

[Jan Alexander Steffens [:heftig]]

Right. When e10s is disabled (either globally or in a non-e10s window) links are saved by alt+click as expected.

Comment 6

[Jan Alexander Steffens [:heftig]]

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

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: 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

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: 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

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1109146 - Support click target "save" for remote content. r?mak

Bug 1109146 - Support click target "save" for remote content. r?mak

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: 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 11

[:Felipe Gomes (needinfo for replies!)]

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

Comment 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/19257/#review18053

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

ditto for making this a proper javadoc

Comment 14

[:Felipe Gomes (needinfo for replies!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2cc105233ff

Comment 15

[Marco Bonardo [:mak]]

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.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

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

Comment 17

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 18

[Alice0775 White]

Error in Browser console:

openUILink/openLinkIn was called with where == 'save' but without initiatingDoc.  See bug 814264. utilityOverlay.js:227:0

Comment 19

[Justin [:JW_SoftvisionQA]]

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.

Comment 20

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 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/

Comment 21

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 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 22

[Jimmy Wang (:jimicy) - works on e10s stuff]

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 23

[Jimmy Wang (:jimicy) - works on e10s stuff]

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/

Comment 24

[Jimmy Wang (:jimicy) - works on e10s stuff]

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/

Comment 25

[Jimmy Wang (:jimicy) - works on e10s stuff]

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

Comment 26

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 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/

Comment 27

[Jimmy Wang (:jimicy) - works on e10s stuff]

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 28

[Jimmy Wang (:jimicy) - works on e10s stuff]

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 29

[Jimmy Wang (:jimicy) - works on e10s stuff]

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.

Comment 30

[:Paolo Amadini]

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

Comment 31

[Jimmy Wang (:jimicy) - works on e10s stuff]

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 32

[Jimmy Wang (:jimicy) - works on e10s stuff]

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 33

[Jimmy Wang (:jimicy) - works on e10s stuff]

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/

Comment 34

[:Felipe Gomes (needinfo for replies!)]

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

Comment 35

[:Felipe Gomes (needinfo for replies!)]

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 36

[:Felipe Gomes (needinfo for replies!)]

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

Comment 37

[:Felipe Gomes (needinfo for replies!)]

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

Comment 38

[Jimmy Wang (:jimicy) - works on e10s stuff]

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/

Comment 39

[Jimmy Wang (:jimicy) - works on e10s stuff]

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 40

[Jimmy Wang (:jimicy) - works on e10s stuff]

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 41

[Jimmy Wang (:jimicy) - works on e10s stuff]

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 42

[Jimmy Wang (:jimicy) - works on e10s stuff]

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 43

[Jimmy Wang (:jimicy) - works on e10s stuff]

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/

Comment 44

[Jimmy Wang (:jimicy) - works on e10s stuff]

Please check in Bug 1275289 first.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0008e250eb38&selectedJob=24794560

Comment 45

[Pulsebot]

Pushed by [e-mail]:
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

Comment 46

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

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

Comment 47

[William Hsu [:whsu]]

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

Comment 48

[Jimmy Wang (:jimicy) - works on e10s stuff]

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

Comment 49

[Jimmy Wang (:jimicy) - works on e10s stuff]

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/6-7/

Comment 50

[Jimmy Wang (:jimicy) - works on e10s stuff]

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/7-8/

Comment 51

[Jimmy Wang (:jimicy) - works on e10s stuff]

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/4-5/

Comment 52

[Jimmy Wang (:jimicy) - works on e10s stuff]

Fixed failing test.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d527e9fa5c7&selectedJob=26067870

Check in Bug 1275289 first.

Comment 53

[Pulsebot]

Pushed by [e-mail]:
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

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1915e2586177
https://hg.mozilla.org/mozilla-central/rev/86872f510751
https://hg.mozilla.org/mozilla-central/rev/8e2c02a74e75

Comment 55

[Saheda Reza [:Antora]]

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

Comment 56

[nagarajnaidu921]

[bugday-20170118] Bug verified