820491 - PBM - Unable to initiate a download in Private Browsing mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently, when one attempts to initiate a download in a private tab, nothing seems to happen. No Gecko exceptions are shown in console.


STR:

i) Open a new private tab
ii) http://nightly.mozilla.org
iii) Attempt to download Nightly for Android

Verify that you can do so normally in a regular tab

--
Nightly (12/11)
Samsung Galaxy Nexus (Android 4.1.2)

Comment 1

[Josh Matthews [:jdm]]

Presumably the code that interacts with nsIDownloadManager needs to be updated to use GUIDs (getDownloadByGUID, aDownload.guid), and things like activePrivateDownloads and  activePrivateDownloadCount. Note that new methods like aDownload.retry/remove/pause/cancel can help with the guid conversion if you already have a download object.

Comment 2

[Josh Matthews [:jdm]]

Also addPrivacyAwareListener instead of addListener.

Comment 3

[Josh Matthews [:jdm]]

And after reading http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutDownloads.js, http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/downloads.js and http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DownloadManagerUI.js, looks like it needs to access the privateDBConnection as well.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Josh is this something you can take  care of?

Comment 5

[Josh Matthews [:jdm]]

Attachment: Update download manager code to work with private downloads.

This is completely untested, as I don't have a mobile build set up, nor do I possess a device that can run Fennec. However, this should be the lion's share of the work.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Update download manager code to work with private downloads.

And of course I forgot to qref.

Comment 7

[Josh Matthews [:jdm]]

One thing to note is that right now the downloads page will show you either public downloads or private ones, depending on what kind of page you're looking at when you launch it (theoretically, at least). You might want to just create a combined view and observe the last-pb-context-exited notification and rebuild the view if it occurs.

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 692084 [details] [diff] [review]
Update download manager code to work with private downloads.

(In reply to Josh Matthews [:jdm] from comment #7)
> One thing to note is that right now the downloads page will show you either
> public downloads or private ones, depending on what kind of page you're
> looking at when you launch it (theoretically, at least). You might want to
> just create a combined view and observe the last-pb-context-exited
> notification and rebuild the view if it occurs.

Yeah, I think that's how we'll want the downloads page to work. There's some errors with the patch as-is, but I can take over from here - thanks for doing this much!

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Update download manager code to work with private downloads

Hijacked jdm's patch; fixed the errors, made about:downloads show a combined view, and uses alternate styling for private downloads. I kept the downloads list separated by showing all private downloads above the non-private ones.

Test build here: http://dl.dropbox.com/u/35559547/fennec-pb-downloads.apk

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Fix DownloadManagerUI component

As jdm pointed out, our DownloadManagerUI implementation is pretty broken. This fixes it.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Fix DownloadManagerUI component, v2

Minor fixes to JSDoc syntax.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 694142 [details] [diff] [review]
Part 1: Update download manager code to work with private downloads

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

This breaks the "No downloads" message.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +418,5 @@
>    _removeItem: function dl_removeItem(aItem) {
>      // Make sure we have an item to remove
>      if (!aItem)
>        return;
>    

White space here and below.

@@ +449,5 @@
> +      aDownload.remove();
> +      try {
> +        if (f) f.remove();
> +      } catch (ex) {
> +        console.log("Error: removeDownload() " + ex);

Leave this logging in. If you want to hide it in private mode (I assume that console.log has no idea what's private and what's not in this context), that's fine. Same for other logging in here. Maybe a helper logForDownload(aDownload, aMessage, aException)?

::: mobile/android/chrome/content/aboutDownloads.xhtml
@@ +28,5 @@
>      <div>&aboutDownloads.header;</div>
>    </div>
> +  <ul id="downloads-list">
> +    <li id="private-divider" />
> +  </ul>

This will break the "No downloads" message, which we currently show using the all too clever

#downloads-list:empty + #no-downloads-indicator { display: block; }

If we want these in a separate list, I'd just add a new UL for it and update the selector to check both.

::: mobile/android/themes/core/aboutDownloads.css
@@ +63,3 @@
>  }
>  
> +li.normal {

I don't think we need "normal". Its either private or not.

@@ +99,5 @@
> +  background-image: none;
> +  color: #000;
> +}
> +
> +li:active .details {

This is changing from what we showed before, right?

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Update download manager code to work with private downloads, v2

Moved logging to a utility function and separated the private/normal downloads into separate lists.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Whitespace cleanup in aboutDownloads.js

There's lots of more whitespace in that file besides what was pointed out in comment 12, so removes it all.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 694549 [details] [diff] [review]
Part 1: Update download manager code to work with private downloads, v2

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

Looks and works good. Some little nitpick things.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +179,5 @@
> +  observe: function (aSubject, aTopic, aData) {
> +    let privateDownloads = this._privateList.children;
> +    for (let i = 0; i < privateDownloads.length; ++i) {
> +      this._privateList.removeChild(privateDownloads[i]);
> +    }

Can you just do this._privateList.innerHTML = ""; ?

@@ +372,4 @@
>      }
>    },
>  
> +  getDownloads: function dl_getDownloads(isPrivate) {

I don't like this boolean. As a caller, the API doesn't make any sense at a glance. I think you're better to allow passing an optional aType string (default to "") and then in isPrivate you can check aType == "private" instead.

@@ +512,5 @@
> +      }
> +    }.bind(this));
> +  },
> +
> +  _updateDownloadRow: function dl_updateDownloadRow(aItem, aDownload){

nit: space before {

::: mobile/android/chrome/content/downloads.js
@@ +22,5 @@
>        return;
>      this._initialized = true;
>  
>      // Monitor downloads and display alerts
> +    this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);

Make this lazy.

@@ +24,5 @@
>  
>      // Monitor downloads and display alerts
> +    this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> +    this._progressAlert = new AlertDownloadProgressListener();
> +    this._dlmgr.addPrivacyAwareListener(this._progressAlert);

You might as well make this lazy too.

::: mobile/android/themes/core/aboutDownloads.css
@@ -45,5 @@
>  }
>  
>  li {
>    color: black;
> -  background-image: url("chrome://browser/skin/images/row-bg-normal.png");

I'd rather just leave these rules and just have the special rules for private downloads. i.e. If we add a third list it should look fine even if we don't style it.

@@ +59,5 @@
>  
> +#normal-downloads-list li {
> +  background-image: url("chrome://browser/skin/images/row-bg-normal.png");
> +  color: #000;
> +}

That means you don't need this

@@ +72,5 @@
> +}
> +
> +#private-downloads-list div.details {
> +  background-color: #393e43;
> +}

And you can move the other details bit up and just use it here.

@@ +87,2 @@
>    background-image: none;
> +  background-color: transparent;

And this should be:

li:active,
#private li:active { }.

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 694147 [details] [diff] [review]
Part 2: Fix DownloadManagerUI component, v2

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

::: mobile/android/components/DownloadManagerUI.js
@@ +20,5 @@
>    show: function show(aWindowContext, aID, aReason) {
>      if (!aReason)
>        aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
>  
> +    this._getBrowserApp().selectOrOpenTab("about:downloads");

How sure do you feel that _getBrowserApp() will always return something and not throw? This could potentially be called early in startup before our window was open, or during shutdown....

Maybe we want to throw in those cases though?

Comment 17

[Wesley Johnston (:wesj)]

jdm, It looks like this stuff requires us to make a connect to the dlmanager at startup (so that we can attach some listeners and things). File I/O/memory/etc being as bad as it is on some phones, we've always been really careful in Fennec to not start services until we need them.

We previously just added an observer for dl-start notifications and didn't bother to initialize any of this until we heard from that (i.e. most browsing sessions we'd hear nothing and not bother with this).

How confident are you that this is safe/ok to do during startup?

Comment 18

[Josh Matthews [:jdm]]

It's going to be reading in the downloads SQL database from disk when the service is initialized. One thing to note, though, is that unless the service is initialized at startup, any in-progress downloads from the last session won't resume until the service is initialized. I don't really know what your other options here are.

Comment 19

[Wesley Johnston (:wesj)]

(In reply to Wesley Johnston (:wesj) from comment #15)
> > +    this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> 
> Make this lazy.

I was wrong that we can't just make this lazy. I talked to mfinkle and I think for now we're just best to land this bit as is and see how big the startup hit is. If there is a significant one, we can start looking at ways to fix it.

Thanks.

Comment 20

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b390365303
https://hg.mozilla.org/integration/mozilla-inbound/rev/27c858f88513
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6684011028

Comment 21

[[e-mail]]

Comment on attachment 694147 [details] [diff] [review]
Part 2: Fix DownloadManagerUI component, v2

>   show: function show(aWindowContext, aID, aReason) {
show() now has a fourth optional parameter, the privacy status, you could use this to ensure you open a tab in an appropriately private window.

>   get visible() {
[Hmm, there's no way of distinguishing between normal and private visibility...]

Comment 22

[(no longer active)]

Neil, please file follow-up bugs for these types of issues.  Commenting to indicate problems in a fixed bug almost always goes unnoticed.

Thanks!

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b6b390365303
https://hg.mozilla.org/mozilla-central/rev/27c858f88513
https://hg.mozilla.org/mozilla-central/rev/5d6684011028

Comment 24

[Brian Nicholson (:bnicholson)]

Comment on attachment 694147 [details] [diff] [review]
Part 2: Fix DownloadManagerUI component, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 582244
User impact if declined: downloads can't be started in private browsing mode
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 25

[Brian Nicholson (:bnicholson)]

Comment on attachment 694549 [details] [diff] [review]
Part 1: Update download manager code to work with private downloads, v2

See above

Comment 26

[Brian Nicholson (:bnicholson)]

Comment on attachment 694550 [details] [diff] [review]
Part 3: Whitespace cleanup in aboutDownloads.js

See above

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 694147 [details] [diff] [review]
Part 2: Fix DownloadManagerUI component, v2

Approved. We'll want downloads in pb mode :)

Comment 28

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1359a8d6dec5
https://hg.mozilla.org/releases/mozilla-aurora/rev/ff139893a46e
https://hg.mozilla.org/releases/mozilla-aurora/rev/d053fe60a0a5

Comment 29

[Andreea Pod]

Verified fixed on:
-build: Firefox for Android 20.0b6 (2013-03-21)
-device: Samsung Galaxy Nexus
-OS: Android 4.1.1