Closed Bug 965414 Opened 6 years ago Closed 6 years ago

download silently fails after creating zero-byte file

Categories

(Firefox Graveyard :: Webapp Runtime, defect, P1)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: myk, Assigned: marco)

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

Downloading a file from within a webapp silently fails after creating a zero-length file in the filesystem.

I can reproduce by installing Mykzilla <http://mykzilla.org/app/> and using it to go to <http://mozqa.com/data/firefox/downloading/> and download one of the unknown types (per the steps in similar bug 749047).

And Tucker McKnight sees it on some apps in the Marketplace, per his dev-webapps thread <https://groups.google.com/forum/#!topic/mozilla.dev.webapps/uw3pkUIFDaw>:

> For example, try Mondrian (https://marketplace.firefox.com/app/mondrian).
> Draw a shape, then go to File > Download as PNG. The file that gets downloaded
> will be zero bytes. The same is true for WGER
> (https://marketplace.firefox.com/app/wger-workout-manager), which allows you
> to save PDFs and iCalendar files. It works on Android, but not Mac OSX.

I haven't tested in other OSes yet, but we should check in Linux and Windows to see if we can reproduce it there as well.
I think this may be a regression, because I tested file downloads when I fixed the branding issue (I don't remember the bug number).

Myk, do you see the download manager window?
No, I don't see the download manager window.  I just see the dialog asking me to confirm the download, and afterward nothing appears to happen (but if I open my Downloads folder I see that a zero-length file has been created in it).
Keywords: regression
Assignee: nobody → dpreston
Priority: -- → P1
This was the bug (fixed in 27.0):
https://bugzilla.mozilla.org/show_bug.cgi?id=851217

Now we have the dialog window for downloading, but only 0 bytes files.
I have experienced this with 27.0 and 27.0.1 with the OpenSolaris version compiled by the Oracle Desktop team. I have tried starting in safe mode but still get 0 byte files in both the Gnome desktop and from terminal. ie: -rw-r--r-- 1 fred staff     0 Mar  9 07:10 Weather-Merit-Badge-Lesson-Plan-Small-Groups.doc
(In reply to choope1 from comment #4)
> I have experienced this with 27.0 and 27.0.1 with the OpenSolaris version
> compiled by the Oracle Desktop team. I have tried starting in safe mode but
> still get 0 byte files in both the Gnome desktop and from terminal. ie:
> -rw-r--r-- 1 fred staff     0 Mar  9 07:10
> Weather-Merit-Badge-Lesson-Plan-Small-Groups.doc

Have you experienced it with a webapp or with Firefox itself?
With Firefox. Right click, Save Link As...
(In reply to choope1 from comment #6)
> With Firefox. Right click, Save Link As...

Is the download dialog window being opened?
Yes it does. I also get 0 byte files with 28.0b9.
I guess this is a regression with the toolkit download manager.
Flags: needinfo?(paolo.mozmail)
I have looked into this but did not get very far. Some pointers about where to look would be helpful.
Don't know why (sorry about my rant here) but this is a big problem to use firefox for webapps. First we had the problem with the download window (not showing at all). Now we do see the window but all the downloaded files have 0 bytes length. I've tested this in Windows and Ubuntu. No matter of the file type (.csv, .pdf) all files are empty. The bug with the download window was reported in firefox 24.0. Now we are waiting for 28.0 and still no fix for this.
It's difficult to say where the problem could be. Are you able to reproduce the issue while debugging?

Not strictly related to this bug, but it's worth considering to migrate to Downloads.jsm as the back-end (bug 911636). We don't run most of the old regression tests on the Toolkit Download Manager anymore, while the new module has better coverage. The migration probably requires writing a new front-end window, but it is easier than it seems (you don't need all the backwards compatibility machinery we had in the Downloads Panel on Desktop).
Flags: needinfo?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #12)
> It's difficult to say where the problem could be. Are you able to reproduce
> the issue while debugging?

The issue is always reproducible.
I see the following error messages whenever I try to download a file:
[19483] WARNING: The dialog should nullify the dialog progress listener: file /home/marco/Documents/FD/src/uriloader/exthandler/nsExternalHelperAppService.cpp, line 2147
[19483] WARNING: NS_ENSURE_TRUE(!mUseJSTransfer) failed: file /home/marco/Documents/FD/src/toolkit/components/downloads/nsDownloadManager.cpp, line 1557
[19483] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /home/marco/Documents/FD/src/toolkit/components/build/../downloads/nsDownloadProxy.h, line 49
[19483] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /home/marco/Documents/FD/src/uriloader/exthandler/nsExternalHelperAppService.cpp, line 2169

I think the problem is that when we build Firefox, we build with MOZ_JSDOWNLOADS defined, so mUseJSTransfer is true.
The webapp runtime is built with Firefox, so MOZ_JSDOWNLOADS is defined for the webapprt too, mUseJSTransfer is true but we don't have the JS implementation.
Looks like this is indeed the problem. Building with MOZ_JSDOWNLOADS undefined fixes the problem.
Attached patch Patch (obsolete) — Splinter Review
Is using the pref instead of MOZ_JSDOWNLOADS reasonable?

I will create a webapprt test in another patch.
Assignee: dpreston → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8406377 - Flags: feedback?(paolo.mozmail)
Attached patch Test (obsolete) — Splinter Review
Here's the test. It is valid no matter which approach we take to fix the issue.
Attachment #8406487 - Flags: review?(myk)
Comment on attachment 8406377 [details] [diff] [review]
Patch

We defined the MOZ_JSDOWNLOADS build-time constant in bug 928349 as a way to reduce the maintenance surface of Firefox, by ensuring that the old API cannot be activated by users or add-ons manually flipping the preference, which would cause a partially broken and insecure experience.

Since Firefox for Metro used the old API and was also built with Firefox, we used the IsRunningInWindowsMetro internal check to create an exception when running in the Metro environment, while keeping the non-overridability in Firefox.

We could use a similar technique here for the moment, with the understanding that once bug 901360 is resolved we'll have no more hard blockers to removing the old API. We'll provide enough lead time, but eventually the Webapp Runtime and Firefox for Metro will need to switch to the new API.
Attachment #8406377 - Flags: feedback?(paolo.mozmail)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8406377 - Attachment is obsolete: true
Attachment #8407640 - Flags: review?(paolo.mozmail)
Comment on attachment 8407640 [details] [diff] [review]
Patch v2

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

Thanks for the patch!

If you can make it so that the initialization code is not compiled on B2G and Android when MOZ_JSDOWNLOADS is defined, it would be nice, but not required as we're working on removing the whole component anyways.
Attachment #8407640 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8406487 [details] [diff] [review]
Test

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

This looks good and passes with the fix for bug 1001681.

::: webapprt/test/chrome/browser_download.js
@@ +39,5 @@
> +    }
> +  },
> +  onStateChange: function(a, b, c, d, e) { },
> +  onProgressChange: function(a, b, c, d, e, f, g) { },
> +  onSecurityChange: function(a, b, c, d) { }

Nit: you don't need to specify the parameters for these stub functions; but if you do specify them, then you might as well use recognizable names for them.
Attachment #8406487 - Flags: review?(myk) → review+
Attached patch PatchSplinter Review
Carrying r+.
Attachment #8407640 - Attachment is obsolete: true
Attachment #8413418 - Flags: review+
Attached patch TestSplinter Review
Carrying r+.
Attachment #8406487 - Attachment is obsolete: true
Attachment #8413419 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/69fa0b8c4de6
https://hg.mozilla.org/mozilla-central/rev/6aa938a57e1a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.