Closed Bug 1128480 Opened 5 years ago Closed 5 years ago

Download fails while e10s is disabled and the original download page is changed

Categories

(Firefox :: File Handling, defect)

38 Branch
defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
e10s - ---
firefox38 --- verified

People

(Reporter: VarCat, Assigned: Gijs)

References

Details

Attachments

(1 file, 3 obsolete files)

Environment:

FF 35,38
OS: win 7 x64

STR:

1. Install FF35.
2. Set Downloads "Always ask me where to save files" from about:preferences.
3. Go to http://www.apple.com/it/itunes/download/ and press download.
4. Wait untill the page is forwarded to http://www.apple.com/it/confirm/itunes/thankyou.html
5. Press Save File.
First Issue:
The download fails with this error:

[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 258"  data: no] Promise-backend.js:868

6. Close FF 35.
7. Start FF 38 using the same profile.
8. Go to http://www.apple.com/it/itunes/download/ and press download.
9. Wait untill the page is forwarded to http://www.apple.com/it/confirm/itunes/thankyou.html
10. Press Save File.

Issue:
The download is failing and this error is 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
NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITaskbarTabPreview.invalidate] WindowsPreviewPerTab.jsm:401:0


Pleas note that this issue is intermittent and it reproduces around 20% of time.
Flags: needinfo?(gijskruitbosch+bugs)
Are you sure this doesn't happen on a vanilla profile, if you say it reproduces only 20% of the time? And does it only happen on Windows? Does e10s being on/off make a difference?
Flags: needinfo?(catalin.varga)
You are absolutely correct my assumption was wrong it has to do with e10s, the bug is reproducible only with e10s disabled and has nothing to do with using FF35 prior to FF38, this are the actual STR:

1. Start FF38 using a new profile.
2. Set "Always ask me where to save files".
3. Disable e10s.
4. Go to http://www.apple.com/it/itunes/download/ and press download.
5. Wait until the page is forwarded to http://www.apple.com/it/confirm/itunes/thankyou.html 
6. Press Save File.
Flags: needinfo?(catalin.varga)
Summary: Profile gets corrupted after a failed download → Download fails while e10s is disabled and the original download page is changed
I can't reproduce this at all. Catalin, can you try to work on more detailed instructions that reproduce more than 20% of the time? (Tried reproducing on both Windows and OS X, both with e10s on)

(or does this require the profile to be old after all?)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(catalin.varga)
The bug is not reproducible with e10s enabled only with e10s disabled my assumption that the bug is due to using an older profile was wrong. The steps used to reproduce the bug are found on comment 2 . The bug is reproducible on Mac Os X also.
Flags: needinfo?(catalin.varga)
Hrmpf. This is what I get for writing a unit test but no integration test (which is not really possible here because of the system dialogs involved, I think, but either way, it's annoying).

I'll have to look into this, because non-e10s is still what we're shipping everywhere besides Nightly, and AFAICT this means bug 1115248 just isn't really fixed on non-e10s (yet).
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1115248
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Points: --- → 5
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+
OS: Windows 7 → All
Hardware: x86_64 → All
So here the issue is that when nsHelperAppDlg.js gets called, the window has already disappeared... :-(

Digging here, I think:

- nsExternalHelperAppService calls the dialog from RequestSaveDestination  ( http://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/uriloader/exthandler/nsExternalHelperAppService.cpp#l2217 )
- which only happens if we call SaveToDisk ( http://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/uriloader/exthandler/nsExternalHelperAppService.cpp#l2253 )
- the comment there is a lie, and saveToDisk can be called both from the nsExternalHelperAppService itself and from nsHelperAppDlg.js, but we only hit the broken "where's my context" path if it's called from the dialog - if it's called from nsExternalHelperAppService itself that's done here: http://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/uriloader/exthandler/nsExternalHelperAppService.cpp#l1757 which is sync from OnStartRequest, which means the context can't be stale.
- if it's called from the dialog, that means we've effectively hit the other branch in the last link, ie alwaysAsk is true and we call .show() on the dialog, with the context, which is for definite alive at that point. In fact, we already save it as mContext in the dialog code (which won't stop it from going stale as it's just an interface requestor, not a window ref).

IOW: I think I can fix this by getting the download dir helper + parent ref in the show() method and using those if the getInterface call fails based on the aContext we get passed in promptForSaveToFileAsync. I should probably weakref the window so we don't hang on to it and leak it, and just use a |null| parent if the window is gone by the time we hit promptForSaveToFileAsync - after all, it doesn't necessarily make sense to even parent the dialog at that point, as the window we /should/ be parenting it to is gone and we might mislead the user if we parent it to the same outer window (e.g. offer download from evil.com, redirect to angelic.com, hey, look at me...).

I'll work on a patch (and test!) that does this, assuming it holds up when I actually implement it. :-)
I have a patch and a test. The problem is that the test doesn't fail without the patch, because in my test the context window doesn't get destroyed. I don't understand what is special about the iframe Apple uses to download iTunes that forces it to go away more quickly.

Boris, can you help enlighten me? (for steps, see comment #2, try in nightly with e10s off)
Flags: needinfo?(bzbarsky)
Assuming I can fix this up to actually fail without the patch I just added...
Attachment #8561576 - Flags: feedback?(felipc)
The only think I can think of offhand is that window.close() is kinda async.  Have you tried actually using an iframe in the test and just removing it from the DOM to trigger docshell/frameloader/etc teardown?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #10)
> The only think I can think of offhand is that window.close() is kinda async.
> Have you tried actually using an iframe in the test and just removing it
> from the DOM to trigger docshell/frameloader/etc teardown?

Yeah, the test patch here uses an iframe, and then navigates the parent (which is what apple does, too, AFAICT...).

I can try nuking the iframe separately, instead of navigating the parent, if that sounds more plausible to hit the same case as Apple is hitting...
Navigating the parent might bfcache it or whatnot.  Probably safer to try removing the iframe from the DOM....
Attachment #8561576 - Attachment is obsolete: true
Attachment #8561576 - Flags: feedback?(felipc)
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Attachment #8561571 - Flags: review?(felipc) → review+
Attachment #8561676 - Flags: review?(felipc) → review+
btw, any idea why this didn't happen with e10s enabled? I wonder if that means that there's a leak with e10s somewhere preventing the iframe to go away
(In reply to :Felipe Gomes from comment #14)
> btw, any idea why this didn't happen with e10s enabled?

No, I'm not sure. That'd require more testing, and I don't understand e10s well enough to figure out why there'd be a difference.

> I wonder if that
> means that there's a leak with e10s somewhere preventing the iframe to go
> away

Naively, at least, this sounds possible to me, but I wouldn't know how to test this hypothesis, and don't really have cycles to dive in.


trypush:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=5500837fcee0
(In reply to :Gijs Kruitbosch from comment #15)
> trypush:
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5500837fcee0

This is orange. I should have purged the pageshow listener completely. I'd land that right now, or trypush it, but all trees are closed...
Attachment #8561571 - Attachment is obsolete: true
Attachment #8561676 - Attachment is obsolete: true
Comment on attachment 8564167 [details] [diff] [review]
Patch for checkin

Carrying over r+
Attachment #8564167 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/688a64d4e746
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
QA Contact: catalin.varga
Flags: in-testsuite? → in-testsuite+
Verified as fixed using:

FF 39
BUild Id: 20150226030225
Os: Win 7 x64, Mac Os X 10.9.5
Status: RESOLVED → VERIFIED
Mistakenly filed against Firefox 38 and should be instead 38 Branch. Sorry for the spam. dkl
Version: Firefox 38 → 38 Branch
Back in FF41 w8 (iTunes)
Depends on: 1196144
You need to log in before you can comment on or make changes to this bug.