646262 - Dont dig into the download manager database directly

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: Patch

While fixing Bug 645015 I noticed that we are messing with the download manager sqlite database directly in Fennec in order to "fake" the download of a PDF. However, since we don't have progress events to listen to, we have resorted to hacking the sqlite ourselves.

One alternative might be to QI the download to a nsIWebProgressListener and sending it a STATE_STOP + STATE_IS_NETWORK state:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.cpp#2471

This seems to work for me. We loose the ability to set the referrer on the download, so "Go to page" in the download manager no longer works. Other than that, things seem fine. But it makes our implementation less dependent on the structure of the underlying database, which seems good.

Alternatively, we could look at the changing nsIDownloadManager to include either methods to "end" a download, or modify addDownload so that it can create already complete download entries.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 522849 [details] [diff] [review]
Patch

>diff --git a/chrome/content/common-ui.js b/chrome/content/common-ui.js

>     stmt.params.target = Services.io.newFileURI(file).spec;
>     stmt.params.startTime = Date.now() * 1000;
>     stmt.params.endTime = Date.now() * 1000;
>     stmt.params.state = Ci.nsIDownloadManager.DOWNLOAD_NOTSTARTED;
>     stmt.params.referrer = current;
>     stmt.execute();
>     stmt.finalize();

Why do we need the above code?

>-    let newItemId = db.lastInsertRowID;
>-    let download = dm.getDownload(newItemId);
>+    var downloadManager = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
>+    let download = downloadManager.addDownload(Ci.nsIDownloadManager.DOWNLOAD_TYPE_DOWNLOAD, current,
>+                                               Services.io.newFileURI(file), fileName, null,

I thought "current" was a string

>     let data = {
>       type: Ci.nsIPrintSettings.kOutputFormatPDF,
>-      id: newItemId,
>-      referrer: current,
>+      id: download.id,
>+      referrer: current.spec,

I thought "current" was a string?

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Real v1 Patch

Whoops. Really sorry. Mistake unbitrotting when I split this part of the patch off of the one in bug 645015.

current actually needs to be an nsIURI here so that it can be passed into the addDownload method.

Comment 3

[Shawn Wilsher :sdwilsh]

I'll try to get this feedback request done by Monday.  It might happen today even.  Sorry for the delay.

Comment 4

[Wesley Johnston (:wesj)]

sdwilsh ping?

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> sdwilsh ping?
Sorry; I've been super busy.  I plan to get through my queues today (for real).

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This sounds much much more cleaner to me, I'm just sad that we will loose the referrer :(

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> This sounds much much more cleaner to me, I'm just sad that we will loose the
> referrer :(

The more I think about it, I don't want to lose the referrer. We use that to open the page where the download happened. Kinda useful. We should wait for a real solution to this.

Shawn's feedback will be helpful

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Shawn's feedback will be helpful
...and I will do it today!  I promise!

Comment 9

[Shawn Wilsher :sdwilsh]

OK, how are you currently downloading the pdf file anyway?  I think there is even a less hacky way to make this work...

Comment 10

[Wesley Johnston (:wesj)]

We are currently adding the download to the download manager (by manually editing the sqlite):

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/common-ui.js#248

Then we fire a message off to the child process which is received here:

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/content.js#477

That basically prints to the pdf. Then it sends a second message back up to the parent to say that it is done/succeeded, which then updates the download manager:

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser-ui.js#1006

Comment 11

[Shawn Wilsher :sdwilsh]

I don't think I was being clear; sorry.  Specifically, where is the file actually downloaded?  Is it done in the content process, or the chrome process?

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 522925 [details] [diff] [review]
Real v1 Patch

So you actually have the right idea here :)

>+++ b/chrome/content/browser-ui.js
>+          download.QueryInterface(Ci.nsIWebProgressListener);
You shouldn't have to do the QI here since nsIDownload inherits from nsITransfer, which inherits from nsIWebProgressListener64, which inherits from nsIWebProgressListener.

>+          download.onStateChange(null, null,
>+                                 Ci.nsIWebProgressListener.STATE_STOP + Ci.nsIWebProgressListener.STATE_IS_NETWORK,
>+                                 Components.results.NS_OK);
However, if you want the referrer, before you indicate that it has stopped, you actually want to call download.onProgressChange64(null, mockChannel, dlFile.size(), dlFile.size(), dlFile.size(), dlFile.size());  mockChannel needs to be defined like this:
let mockChannel = {
  QueryInterface: XPCOMUtils.generateQI([Ci.nsIRequest, Ci.nsIChannel, Ci.nsIPropertyBag2]),
  getPropertyAsInterface: function(prop, iid)
  {
    if (prop == "docshell.internalReferrer" && iid == Ci.nsIURI) {
        return NetUtil.newURI(json.referrer);
    }
  },
};
This is all untested, but looking at the code, I think this will work.

>+++ b/chrome/content/common-ui.js
>+    let download = downloadManager.addDownload(Ci.nsIDownloadManager.DOWNLOAD_TYPE_DOWNLOAD, current,
>+                                               Services.io.newFileURI(file), fileName, null,
Just do NetUtil.newURI here.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch v2

The progress trick works fine (with some other little tweaks). Also had to add a bit of code to handle referrers changing during the download.

Now I occasionally see the download manager get a bit confused and either
1.) delayedInit not being called, or
2.) delayedInit being called, deleting the active download in the list, and not re-adding it (some sort of race condition?).

Since we are building that list with another set of calls into the database, I'd like to remove them as well, so I'm digging a bit more to look for ways to do that as well as clean up whatever race condition is going on.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> Since we are building that list with another set of calls into the database,
> I'd like to remove them as well, so I'm digging a bit more to look for ways to
> do that as well as clean up whatever race condition is going on.
Apart from active downloads, we don't have an API for that, sadly.

Comment 15

[Wesley Johnston (:wesj)]

Would you be opposed to adding them? And I guess if I am going to go that far, adding an additional API for creating these "fake" downloads?

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> Would you be opposed to adding them? And I guess if I am going to go that far,
> adding an additional API for creating these "fake" downloads?
While I wouldn't be opposed to adding them, I'm not sure it's worthwhile.  Rolling your own SQL for reads is pretty safe, and the download manager UI does it too.

As for "fake" downloads like this, I'm less inclined to add an API (although you could add the details yourself via nsIDownloadHistory and just display download history via places API calls).

Comment 17

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 20

[BMO Automation]

Closing all opened bug in a graveyard component