420230 - unable to save data urls to disk

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

[Seth Spitzer]

unable to save data urls to disk

steps to reproduce

1)  load the data url above
2)  attempt to save to disk

I get the download manager, but the file is never saved.

first spotted this with dolske's APNG Editor addon

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre

Comment 1

[Justin Dolske [:Dolske]]

Hmm, with APNG edit's "Save Animation" and a current nightly on OS X (Gecko/2008022804), these both WFM. Windows-specific bug?

Comment 2

[Seth Spitzer]

I updated to make sure it wasn't fixed recently, and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022806 Minefield/3.0b4pre has the same problem.

Comment 3

[Shawn Wilsher :sdwilsh]

ugh - edward - didn't we fix this once already?

Comment 4

[Ed Lee :Mardak]

Works for me with the provided data uri and data:text/html,Hello World!

Comment 5

[Seth Spitzer]

the hello world data url from edward works for me, but the png data url does not.

edward, when you save that data url to disk, what do you see in the download manager?

I'm attempting to save it as "test.png", so I see

test.png
2.2 KB - data resource

Comment 6

[Seth Spitzer]

an interesting data point:  entries for the files that I'm saving are showing up in "My Recent Documents" under the Start menu (on Windows XP), which makes me think that I am saving them, but then they are getting deleted?

Comment 7

[Ed Lee :Mardak]

So you /do/ see the file show up as 

image.png  <time>
2.2 KB - data resource

?

If you double click the row from the download manager, does it open?

Comment 8

[Seth Spitzer]

> If you double click the row from the download manager, does it open?

"Open" is disabled, and "Open In Containing Folder" gives me the windows explorer alert that the file does not exist or is not a directory.

Comment 9

[Martijn Wargers (dead)]

I noticed this when trying to save the data uri from bug 163971.
If I have a previously saved file with the same name (using a branch build), then that file is being deleted, but the new file is not saved.
It seems as if the download manager is trying to create a directory, instead of a file or something and when that doesn't work, it bails out completely.

Comment 10

[Ed Lee :Mardak]

This seems to be windows only. I wonder if it's related to bug 416683. Because when I toggle browser.download.manager.scanWhenDone to false, I can save okay on windows.

Comment 12

[Jim Mathies [:jimm]]

bug 422920 was marked as blocking / P2.

Comment 13

[Jim Mathies [:jimm]]

Attachment: canvas testcase from 422920

Comment 14

[Jim Mathies [:jimm]]

Attachment: download scanner patch v.1

This patch skips off providing the source uri in cases where the source is a data scheme. Scanning though will still take place on the resulting local file.

Comment 15

[Rob Arnold [:robarnold]]

Comment on attachment 311103 [details] [diff] [review]
download scanner patch v.1

Looks pretty straightforward to me. No threading issues.

Comment 16

[Jim Mathies [:jimm]]

I'll add a test for this as well.

Comment 17

[Ed Lee :Mardak]

I can do the test.

Comment 18

[Ed Lee :Mardak]

Well, depends on what kind of test you want ;) Shawn might want an xpcshell one :p I was just doing to take one of the mochitests that adds a download and hits space to retry the download. You can continue. :p

Comment 19

[Jim Mathies [:jimm]]

actuall I'd kinda like to do it myself. :) working on one now based on your suggested approach. I'll post a new patch here in a bit.

Comment 20

[Jim Mathies [:jimm]]

Attachment: download scanner patch v.3

added mochitest test case. I made it a platform independent test since this seems like a good general test to have.

Comment 21

[Shawn Wilsher :sdwilsh]

I'm not complaining since more eye's looking at the code is a good thing, but toolkit doesn't require superreview.

Edward - can you make sure this testcase follows the form you've been going along with (minimizes dependency on setTimeout).  Thanks!

Comment 22

[Ed Lee :Mardak]

Comment on attachment 311162 [details] [diff] [review]
download scanner patch v.3

>+++ toolkit/mozapps/downloads/tests/browser/browser_bug_420230.js
>+  window.setTimeout(finishUp, 500);
Could you base the testcase on either browser_bug_406857 or browser_bug_412360. They start downloads by creating a download in the canceled state and simulating an enter to restart the download.. So we don't need to explicitly set up our own nsIWBP object and all.

Also, those tests don't use a setTimeout(.., 500) which is frowned upon.

Additionally, you want to check if the file exists. Not just that it got to the finished state. Because currently, downloads _are_ finishing but the file disappears.

Comment 23

[Jim Mathies [:jimm]]

> I'm not complaining since more eye's looking at the code is a good thing, but
> toolkit doesn't require superreview.

Isn't it going to need sr+ if I'm planning on asking for approval1.9b5? 

> Could you base the testcase on either browser_bug_406857...

I think that's the one I started with, but took most of that stuff out. :/ Will get it cleaned up and repost.

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> Isn't it going to need sr+ if I'm planning on asking for approval1.9b5? 
Nope, normal rules apply.  Drivers look for proper review, and really like to see tests ;)

Comment 25

[Jim Mathies [:jimm]]

Attachment: download scanner patch v.4

Comment 26

[Ed Lee :Mardak]

Comment on attachment 311184 [details] [diff] [review]
download scanner patch v.4

Typically you request approval after reviews are done.

r=Mardak

>+++ toolkit/mozapps/downloads/tests/browser/browser_bug_420230.js
>+        case dm.DOWNLOAD_FINISHED:
>+          ok(true, "data: scheme uri successfully downloaded");
>+          if (!aDownload.targetFile.exists())
>+            ok(false, "data: scheme uri target does not exist");
>+          else
>+            ok(true, "data: scheme uri target file exists");
No need for the if/else. Just do..

ok(aDownload.targetFile.exists(), "data: uri target file exists");

And just checking, you were able to pass this test on a windows box only after applying the rest of the patch?

Comment 27

[Jim Mathies [:jimm]]

> Typically you request approval after reviews are done.

sorry, jumpd the gun.

> And just checking, you were able to pass this test on a windows box only after
> applying the rest of the patch?

Hmm, good question, I didn't check that. Will test that now.

Comment 28

[Jim Mathies [:jimm]]

Yes it fails. I get a timeout though, I wonder if I've not finished up right - 

chrome://mochikit/content/browser/../browser/toolkit/mozapps/downloads/tests/browser/browser_bug_420230.js
	PASS - data: uri successfully downloaded
	FAIL - data: uri target file exists - chrome://mochikit/content/browser/../browser/toolkit/mozapps/downloads/tests/browser/browser_bug_420230.js
	FAIL - Timed out - chrome://mochikit/content/browser/../browser/toolkit/mozapps/downloads/tests/browser/browser_bug_420230.js

here's the new code:

case dm.DOWNLOAD_FINISHED:
  ok(true, "data: uri successfully downloaded");
  ok(aDownload.targetFile.exists(), "data: uri target file exists");
  aDownload.targetFile.remove(false);
  dm.removeListener(listener);
  finish();
  break;

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311184 [details] [diff] [review]
download scanner patch v.4

a1.9b5=beltzner, I can see the merit in having this for beta, also: tests, yay!

Comment 30

[Jim Mathies [:jimm]]

Attachment: download scanner patch v.5

updated

Comment 31

[Ed Lee :Mardak]

Comment on attachment 311413 [details] [diff] [review]
download scanner patch v.5

>+        case dm.DOWNLOAD_FINISHED:
>+          ok(true, "data: uri successfully downloaded");
>+          ok(aDownload.targetFile.exists(), "data: uri target file exists");
>+          aDownload.targetFile.remove(false);
Ah. You'll need a try/catch around the remove because it'll throw if it can't delete the file. Shawn, that's an issue on windows, yeah? But in any case, with the patch, the test wouldn't hit the timeout anyway.

Comment 33

[Ed Lee :Mardak]

Hold on, do you have your patch from the dependent bug 416683 applied as well? The data uri should be hitting the BLOCKED_POLICY case -- not the FINISHED case with bug 416683 applied. no?

Comment 34

[Ed Lee :Mardak]

Just to clarify:

without bug 416683, without bug 420230: data uri -> finished but deleted
with bug 416683, without bug 420230: data uri -> blocked and deleted
with bug 416683, with bug 420230: data uri -> finished and not deleted

Comment 35

[Jim Mathies [:jimm]]

yeah I'm looking at it....

Comment 36

[Jim Mathies [:jimm]]

ahh, we start off in state cancelled, skipping the use of AddDownload which is where the check takes place. This should be ok. new try/catch patch forthcoming..

Comment 37

[Jim Mathies [:jimm]]

actually, I _should_ be using AddDownload to test this!

Comment 38

[Ed Lee :Mardak]

Yeah you should be testing with AddDownload. Sorry about leading you to RetryDownload. (Should we be checking policy on retry?)

Instead of adding a canceled download then retrying, do something similar to the first test which adds the download.

1) add a listener
2) add the download
3) check for state changes with the listener

you don't actually need the download UI for this either. Shawn, should this be an xpcshell test or is that a SoC thing converting mochi tests to litmus tests if possible! ;)

Comment 39

[Ed Lee :Mardak]

argh. mochi -> xpcshell not litmus :p

Comment 40

[Jim Mathies [:jimm]]

> Yeah you should be testing with AddDownload. Sorry about leading you to
> RetryDownload. (Should we be checking policy on retry?)

Hmm, searching LXR, it looks like we always inject downloads via Add. Retry looks to be tied to the UI only.

Comment 41

[Shawn Wilsher :sdwilsh]

we should use xpcshell whenever it's possible.

Comment 42

[Jim Mathies [:jimm]]

Attachment: download scanner patch v.6

Ok, converted the whole thing over to a unit test, and confirmed failure on trunk, success with this patch applied.

Comment 43

[Shawn Wilsher :sdwilsh]

Comment on attachment 311474 [details] [diff] [review]
download scanner patch v.6

let works in xpcshell now?  sweeet.

drivers: this fixes a nasty little regression on windows.  patch has tests - yay!  This also got a for 1.9 and for 1.9b5 already, but some reworking was done.

Comment 44

[Jim Mathies [:jimm]]

(Note the changes were to the test and not the patch, which remains unchange.)

Comment 45

[Reed Loden [:reed]]

Comment on attachment 311474 [details] [diff] [review]
download scanner patch v.6

No need to get approval again. I'll land this shortly.

Comment 46

[Reed Loden [:reed]]

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.178; previous revision: 1.177
done
Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp,v  <--  nsDownloadScanner.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h,v  <--  nsDownloadScanner.h
new revision: 1.10; previous revision: 1.9
done
RCS file: /cvsroot/mozilla/toolkit/components/downloads/test/unit/test_bug_420230.js,v
done
Checking in toolkit/components/downloads/test/unit/test_bug_420230.js;
/cvsroot/mozilla/toolkit/components/downloads/test/unit/test_bug_420230.js,v  <--  test_bug_420230.js
initial revision: 1.1
done

Comment 47

[Justin Wood (:Callek)]

There were two |case dm.DOWNLOAD_FAILED:| lines pointing at the same failure code in the test here.

I checked in a fix, rs=gavin for that just now.

Checking in toolkit/components/downloads/test/unit/test_bug_420230.js;
/cvsroot/mozilla/toolkit/components/downloads/test/unit/test_bug_420230.js,v  <--  test_bug_420230.js
new revision: 1.2; previous revision: 1.1

Comment 48

[Jim Mathies [:jimm]]

Oops. Thanks Justin.

Comment 49

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032505 Minefield/3.0b5pre and Win Vista nightly as well. I verified using the data URL and the testcase in the bug.

Comment 51

[giorgos papavasileiou]

(In reply to comment #0)
> unable to save data urls to disk
> 
> steps to reproduce
> 
> 1)  load the data url above
> 2)  attempt to save to disk
> 
> I get the download manager, but the file is never saved.
> 
> first spotted this with dolske's APNG Editor addon
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022704
> Minefield/3.0b4pre


You can only download the image,because there is only an image.An web page does not exist.