1115248 - Can't downloads MP3s from Amazon music due to chrome code throwing exceptions

Status: VERIFIED FIXED

(Firefox :: File Handling, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Steps to reproduce:

1)  Be in non-e10s mode.
2)  Log in to Amazon music.
3)  Click "Songs" under "Your Library"
4)  Check the checkbox next to the first song.
5)  Click the Download button.
6)  When the helper app dialog comes up, select "Save File" and click OK.
7)  When the filepicker comes up, just click OK (in my case on the home
    directory).

EXPECTED RESULTS: File starts downloading

ACTUAL RESULTS: No download.

Additional information:

1)  I can't seem to reproduce this in a clean profile, though I haven't tried
    that hard.
2)  I can't seem to reproduce this in e10s mode.
3)  At the failure point, the browser console says:

[Exception... "[JavaScript Error: "can't access dead object" {file: "resource://gre/modules/DownloadLastDir.jsm" line: 176}]'[JavaScript Error: "can't access dead object" {file: "resource://gre/modules/DownloadLastDir.jsm" line: 176}]' when calling method: [nsIContentPrefCallback2::handleCompletion]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource://gre/modules/ContentPrefUtils.jsm :: safeCallback :: line 47"  data: yes]

which is why a clean profile might not reproduce this; afaik whether an object is dead is GC-timing-dependent.

Blake has blame for that ContentPrefUtils bit, but not sure where the real issue here is in terms of someone holding on to a content object after its window is unloaded...

If someone has concrete steps for troubleshooting this, happy to try them.

Comment 1

[:Gijs (he/him)]

The code that breaks here is:

176       let loadContext = this.window
177                             .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
178                             .getInterface(Components.interfaces.nsIWebNavigation)
179                             .QueryInterface(Components.interfaces.nsILoadContext);


So I imagine the window with which the DownloadLastDir was created was destroyed.

AFAICT there are two main callsites here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#214
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#529

The latter AFAICT will always have a toplevel chrome window, which from your steps should still be alive (I think?), but it looks like nsHelperAppDlg gets called from:

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

which does:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.h#253

to obtain what eventually gets passed as aContext to nsHelperAppDlg.js, which *could* be either a content or a chrome window, from my naive looks at it. The content window could be going away considering how much of this code is async.

We should fix that.

... except that it looks like all this code really wants to have a content window in the first place in order to work properly, I think? (so if we get a chrome window we should probably get the content window...). Can/should we just hang on to the load context as soon as we're constructed here? Looks like that's what we care about for almost all the methods anyway... Boris/Paolo, does that make sense? :-\

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> AFAICT there are two main callsites here:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/
> nsHelperAppDlg.js#214
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/
> contentAreaUtils.js#529

To be clear, these are the callsites for the constructor, which is where this.window comes from.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  So I agree that in my case the codepath taken is the one in nsExternalHelperAppService.cpp.  I also agree that it's quite possible Amazon is creating some temporary window for the download (e.g. an iframe that they then remove).

I just checked, and in the DownloadLastDir constructor we're ending up with an about:blank window which has the Amazon Music window as .top.

> ... except that it looks like all this code really wants to have a content window in the > first place in order to work properly, I think?

I _think_ the only uses of the loadcontext stuff are to pass it to ContentPrefService2.jsm stuff, which just wants it for usePrivateBrowsing, right?  I bet that's set on the chrome window too...

> Can/should we just hang on to the load context as soon as we're constructed here? 

We could, but the load context is just the docshell.  Won't that have the same dead-object-proxy issue?

We could improve this particular testcase by setting this.window to aWindow.top, which will at least ensure we're using the toplevel thing in the tab, as opposed to an iframe which can go away.  The tab itself could go away, but that seems like it would happen more rarely.  I'd probably suggest doing this as a quick fix no matter what else we do here.

If the only thing we _really_ need here is the private browsing boolean, then the best thing would be to grab _that_ in the ctor and then change the APIs we're using to take that boolean, not a loadcontext or something...

Comment 4

[:Paolo Amadini]

If Amazon music doesn't work, switch to iTunes... wait, you may run into bug 918000 :-)

And bug 673366 is likely another manifestation of the same root cause.

If both the window and the nsILoadContext may go away, we should probably avoid keeping references to them in the first place and copy just the data we need as soon as possible, before any asynchronous operation. (Channel loading in general is asynchronous, but I don't know at which point the external app handler constructor is invoked. If there is still a slight chance of being too late, it would be unlikely anyways, and maybe we should just abort any processing in that case.)

Ideally we should keep the browser window reference for dialog parenting, and if the load context is used only for getting the private browsing status, maybe we can provide an implementation that only returns the correct value of usePrivateBrowsing, without changing the content preferences APIs?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> If both the window and the nsILoadContext may go away

Note that it's possible the nsILoadContext can't go away today.  I just don't think we should rely on that for the future.

Past that, it all sounds fine to me, but it's easy for me to say if I'm not writing the patch.  ;)

Comment 6

[:Gijs (he/him)]

See also: bug 1013234. It seems we have more code that makes assumptions about windows staying around and then gets very sad about not being able to determine the window's private state asynchronously.

Comment 7

[:Gijs (he/him)]

Attachment: let DownloadLastDir work even if the window it depends on goes away,

The test fails without the code change, but gets different error message (the window object's getInterface calls fail). I don't know why the errors are different, but I verified the testcase from comment #0 manually, and it works with the code change, too, and after spending most of today fighting the tests here, I'm going to say that this test should be enough for now.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8549025 [details] [diff] [review]
let DownloadLastDir work even if the window it depends on goes away,

Review of attachment 8549025 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/test/browser/head.js
@@ +77,5 @@
> +
> +  let timeout = setTimeout(() => {
> +    tab.linkedBrowser.removeEventListener(eventType, handle, true);
> +    deferred.reject(new Error("Timed out while waiting for a '" + eventType + "'' event"));
> +  }, 30000);

won't this trigger the "can't use non-0 setTimeouts" rule? You probably need to use SimpleTest.requestFlakyTimeout("reason"). Although I don't know how that works for head.js files.. (wouldn't that allow flaky tests on all files using it..?)
Too bad we have to keep copying around these helper functions' implementations all around the various head.js files..

Comment 9

[:Gijs (he/him)]

(In reply to :Felipe Gomes from comment #8)
> Comment on attachment 8549025 [details] [diff] [review]
> let DownloadLastDir work even if the window it depends on goes away,
> 
> Review of attachment 8549025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/downloads/test/browser/head.js
> @@ +77,5 @@
> > +
> > +  let timeout = setTimeout(() => {
> > +    tab.linkedBrowser.removeEventListener(eventType, handle, true);
> > +    deferred.reject(new Error("Timed out while waiting for a '" + eventType + "'' event"));
> > +  }, 30000);
> 
> won't this trigger the "can't use non-0 setTimeouts" rule? You probably need
> to use SimpleTest.requestFlakyTimeout("reason"). Although I don't know how
> that works for head.js files.. (wouldn't that allow flaky tests on all files
> using it..?)

Ugh, I thought I did a try push here, which would have covered this. Try claims otherwise. I guess instead I'll just remove the timeout (and push to try before landing this).

> Too bad we have to keep copying around these helper functions'
> implementations all around the various head.js files..

Yes. Bug 1093566. I'll see if I have time to make a start there in this iteration. It's high time.

Comment 10

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=048ecead6045

Comment 11

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/120b108aa176

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/120b108aa176

Comment 15

[Catalin Varga [QA][:VarCat]]

Verified this bug using the http://www.apple.com/it/itunes/download/ page and the following environment:

FF 38
Build Id:20150129030202
OS: Win 7 x64, Mac Os X 10.7.5, Ubuntu 12.04 x32

and the bug is fixed however I've noticed an issue.
When I've used an older Firefox version to reproduce the bug it seems that the profile got corrupted.
Using the same profile with the latest Nightly, the download was still failing with the following error( logged in browser console):

[Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: resource://gre/components/nsHelperAppDlg.js :: nsUnknownContentTypeDialog.prototype.promptForSaveToFileAsync/< :: line 242"  data: no] Promise-backend.js:870:0

Comment 16

[:Gijs (he/him)]

(In reply to Catalin Varga [QA][:VarCat] from comment #15)
> Verified this bug using the http://www.apple.com/it/itunes/download/ page
> and the following environment:
> 
> FF 38
> Build Id:20150129030202
> OS: Win 7 x64, Mac Os X 10.7.5, Ubuntu 12.04 x32
> 
> and the bug is fixed however I've noticed an issue.
> When I've used an older Firefox version to reproduce the bug it seems that
> the profile got corrupted.
> Using the same profile with the latest Nightly, the download was still
> failing with the following error( logged in browser console):
> 
> [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE)
> [nsIInterfaceRequestor.getInterface]"  nsresult: "0x80004002
> (NS_NOINTERFACE)"  location: "JS frame ::
> resource://gre/components/nsHelperAppDlg.js ::
> nsUnknownContentTypeDialog.prototype.promptForSaveToFileAsync/< :: line 242"
> data: no] Promise-backend.js:870:0

Can you please file a new bug with detailed steps on how to reproduce this, and needinfo me? Thank you!

Comment 17

[Catalin Varga [QA][:VarCat]]

Marking the bug as verified, also I've logged 1128480 bug for the issue described on comment 15 .

Comment 18

[Catalin Varga [QA][:VarCat]]

Linking the logged bug 1128480

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yep, works now.  Thanks for fixing this!

Comment 20

[f0rhum]

Issue is back again (FF41 w8 iTunes)