997756 - Regression: Tapping on a download notification doesn't do anything

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[Marco Castelluccio [:marco]]

STR:
1) Download a file
2) Tap a download completed notification

I expected the file to be opened or a new "about:downloads" tab to be shown (so that I could open the file), instead tapping on the notification just dismissed it.

Comment 1

[Marco Castelluccio [:marco]]

Actually, I could reproduce this problem using the release version on a Motorola Moto G (I can't access the phone now, so I don't recall what version of Android it was running, it should be the latest update though)

Comment 2

[Aaron Train [:aaronmt]]

I tested this on Nightly and we regressed opening about:downloads as opposed to on Aurora. I used the stub installer as just a small download example on http://nightly.mozilla.org

Comment 3

[:Margaret Leibovic]

James, do you have time to help us investigate this?

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Aaron Train [:aaronmt] from comment #2)
> I tested this on Nightly and we regressed opening about:downloads as opposed
> to on Aurora. I used the stub installer as just a small download example on
> http://nightly.mozilla.org

So there are two different bugs, let's track here the regression you've found, I'll open another bug for the Moto G problem (if and when I get my hands on it again).

Comment 5

[James Gilbertson]

(In reply to :Margaret Leibovic from comment #3)
> James, do you have time to help us investigate this?

I'll have time after today.

Comment 6

[Wesley Johnston (:wesj)]

Glancing at this, it looks like we just lost this code in the transition. i.e. in onClick here we just log failures to launch:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/DownloadNotifications.jsm#177

whereas our old code we launched aboutDownloads if that failed:

http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/chrome/content/downloads.js#53

We'll have to do some juggling since we don't have a guid anymore.

Comment 7

[James Gilbertson]

(In reply to Wesley Johnston (:wesj) from comment #6)
> Glancing at this, it looks like we just lost this code in the transition.
> i.e. in onClick here we just log failures to launch:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/
> DownloadNotifications.jsm#177
> 
> whereas our old code we launched aboutDownloads if that failed:
> 
> http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/chrome/content/
> downloads.js#53
> 
> We'll have to do some juggling since we don't have a guid anymore.

I suppose one approach would be for DownloadNotification to generate a synthetic id, and pass that in the URL. Then when aboutDownloads.js loads, it could query DownloadNotifications for the actual Download object attached to the id.

Or if it's possible, just pass the Download object itself directly to aboutDownloads when the tab is opened.

Comment 8

[James Gilbertson]

Attachment: Show download in about:downloads if notification fails to launch

Mostly completed patch that fixes the issue.

I'm delaying the call to scrollNotificationIntoView by 100 ms. The reason is that if I call it directly, approximately every 10th load or so it seems to fail, even though all list elements are in the DOM.

I had to stub DownloadIntegration.launchDownload to throw an exception, since I ran into two problems with it:
1) There's no way to tell if it fails to launch the download, since AFAICT it swallows all errors/exceptions.
2) If /storage/sdcard is an actual SD-card, all files will have the x bit set, and it will always display the launch executable file dialog.

root@generic_x86:/ # mount
...
/dev/block/vold/179:1 /mnt/media_rw/sdcard vfat rw,dirsync,nosuid,nodev,noexec,relatime,uid=1023,gid=1023,fmask=0007,dmask=0007,allow_utime=0020,codepage=cp437,iocharset=iso8859-1,shortname=mixed,utf8,errors=remount-ro 0 0

Comment 9

[:Paolo Amadini]

Comment on attachment 8409062 [details] [diff] [review]
Show download in about:downloads if notification fails to launch

This looks good to me!

(In reply to James Gilbertson from comment #8)
> I had to stub DownloadIntegration.launchDownload to throw an exception,
> since I ran into two problems with it:
> 1) There's no way to tell if it fails to launch the download, since AFAICT
> it swallows all errors/exceptions.
> 2) If /storage/sdcard is an actual SD-card, all files will have the x bit
> set, and it will always display the launch executable file dialog.

However, I don't see these changes in this patch.

I think the changes to DownloadIntegration are best addressed in a separate bug. Number 1 might make sense for Desktop as well, and we'll need an xpcshell test for it.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8409062 [details] [diff] [review]
Show download in about:downloads if notification fails to launch

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

::: mobile/android/chrome/content/aboutDownloads.js
@@ +13,4 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "DownloadNotifications", "resource://gre/modules/DownloadNotifications.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise", "resource://gre/modules/Promise.jsm");

We should have real DOM Promises here. Can we use them?

@@ +18,2 @@
>  
> +const strings = Services.strings.createBundle("chrome://browser/locale/aboutDownloads.properties");

Why this change?

::: mobile/android/modules/DownloadNotifications.jsm
@@ +179,5 @@
>  
>    hide: function () {
>      if (this.id) {
>        Notifications.cancel(this.id);
> +      this.previousId = this.id;

You mind adding a little note explaining what previousId is here for?

Comment 11

[James Gilbertson]

Attachment: Show download in about:downloads if notification fails to launch (v2)

I've changed DownloadNotification to just call nsIFile.launch() instead of Download.launch(), due to it's inability to indicate if the launch was successful or not.

about:downloads uses Download.launch(), since it doesn't care about the status of the file launch.

Comment 12

[James Gilbertson]

(In reply to Wesley Johnston (:wesj) from comment #10)
> Comment on attachment 8409062 [details] [diff] [review]
> Show download in about:downloads if notification fails to launch
> 
> Review of attachment 8409062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/aboutDownloads.js
> @@ +13,4 @@
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
> >  
> > +XPCOMUtils.defineLazyModuleGetter(this, "DownloadNotifications", "resource://gre/modules/DownloadNotifications.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", "resource://gre/modules/Promise.jsm");
> 
> We should have real DOM Promises here. Can we use them?

Yes, they work. I've removed the import of Promise.jsm.

> 
> @@ +18,2 @@
> >  
> > +const strings = Services.strings.createBundle("chrome://browser/locale/aboutDownloads.properties");
> 
> Why this change?

I thought for some reason strings were being loaded right away. They're not, so I've reverted the changes in that hunk (other then adding the import for DownloadNotifications).

> ::: mobile/android/modules/DownloadNotifications.jsm
> @@ +179,5 @@
> >  
> >    hide: function () {
> >      if (this.id) {
> >        Notifications.cancel(this.id);
> > +      this.previousId = this.id;
> 
> You mind adding a little note explaining what previousId is here for?

I've added a comment, but I've also reworked this a bit. It's now called knownIds and is a set. When onClick() fails to launch a file, it adds the current id to the set, and then opens about:downloads with that id.

Comment 13

[James Gilbertson]

(In reply to James Gilbertson from comment #11)
> I've changed DownloadNotification to just call nsIFile.launch() instead of
> Download.launch(), due to it's inability to indicate if the launch was
> successful or not.

Figured I should elaborate on this, so I don't forget :)

In DownloadIntegration.launchDownload(), if all else fails, it invokes nsIExternalProtocolService.loadUrl()[1]. That in turn invokes nsIContentDispatchChooser.ask()[2]. However, the return type for ask() is void[3], so there's no way for it to communicate the result of whatever happened.

As a test, I modified the Android implementation of nsIContentDispatchChooser[4] to throw an error if it failed to launch the file, and it seemed to work (however, it does cause console error messages for the exception). It feels hacky though, and I have no idea if that's the correct approach.

1: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#732
2: https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1070
3: https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsIContentDispatchChooser.idl#40
4: https://mxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentDispatchChooser.js#34

Comment 14

[:Paolo Amadini]

(In reply to James Gilbertson from comment #12)
> > We should have real DOM Promises here. Can we use them?
> 
> Yes, they work. I've removed the import of Promise.jsm.

For the record, DOM Promises don't have detection of asynchronous errors in automated tests yet. Since this code is not covered by automated testing, this is probably irrelevant here and DOM Promises are fine as long as you have correct error handling and don't need debugging facilities, but I wanted to give a heads up that Promise.jsm is still preferred until the dependencies of bug 939636 are resolved.

Comment 15

[:Paolo Amadini]

(In reply to James Gilbertson from comment #13)
> (In reply to James Gilbertson from comment #11)
> > I've changed DownloadNotification to just call nsIFile.launch() instead of
> > Download.launch(), due to it's inability to indicate if the launch was
> > successful or not.

Doesn't this always skip the executable warning dialog?

> In DownloadIntegration.launchDownload(), if all else fails, it invokes
> nsIExternalProtocolService.loadUrl()[1]

Ah, it seems that the old code there is also designed to return NS_OK in several case where the permission to open the URL is denied.

> As a test, I modified the Android implementation of
> nsIContentDispatchChooser[4] to throw an error if it failed to launch the
> file, and it seemed to work (however, it does cause console error messages
> for the exception). It feels hacky though, and I have no idea if that's the
> correct approach.

Hm, what did the old nsIDownloadManager-based Android code do in this case? The new API is asynchronous and implemented in JavaScript, but should otherwise do the same basic operations as the old one, maybe we could do something similar to what we previously did.

Comment 16

[James Gilbertson]

(In reply to :Paolo Amadini from comment #15)
> (In reply to James Gilbertson from comment #13)
> > (In reply to James Gilbertson from comment #11)
> > > I've changed DownloadNotification to just call nsIFile.launch() instead of
> > > Download.launch(), due to it's inability to indicate if the launch was
> > > successful or not.
> 
> Doesn't this always skip the executable warning dialog?

It does. I'm not sure how useful it is on Android though. It has it own built-in dialog for installing/upgrading APKs, and nothing else is executable (AFAIK) unless you invoke a shell.

> > In DownloadIntegration.launchDownload(), if all else fails, it invokes
> > nsIExternalProtocolService.loadUrl()[1]
> 
> Ah, it seems that the old code there is also designed to return NS_OK in
> several case where the permission to open the URL is denied.
> 
> > As a test, I modified the Android implementation of
> > nsIContentDispatchChooser[4] to throw an error if it failed to launch the
> > file, and it seemed to work (however, it does cause console error messages
> > for the exception). It feels hacky though, and I have no idea if that's the
> > correct approach.
> 
> Hm, what did the old nsIDownloadManager-based Android code do in this case?
> The new API is asynchronous and implemented in JavaScript, but should
> otherwise do the same basic operations as the old one, maybe we could do
> something similar to what we previously did.

The old implementation just invoked nsIFile.launch().

Comment 17

[:Paolo Amadini]

(In reply to James Gilbertson from comment #16)
> > Doesn't this always skip the executable warning dialog?
> 
> It does. I'm not sure how useful it is on Android though. It has it own
> built-in dialog for installing/upgrading APKs, and nothing else is
> executable (AFAIK) unless you invoke a shell.
> 
> The old implementation just invoked nsIFile.launch().

Sounds fine to me.

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 8411577 [details] [diff] [review]
Show download in about:downloads if notification fails to launch (v2)

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

I just want to use nsIBrowserDOMWindow for opening this (with OPEN_SWITCHTAB so that we reuse an about:downloads tab if we can find one). Its a more stable API. Other than that looks pretty good :) Thanks!

::: mobile/android/chrome/content/aboutDownloads.js
@@ +216,5 @@
> +             .catch(Cu.reportError);
> +    }
> +  },
> +
> +  scrollNotificationIntoView: function (id) {

id -> notificationId (we have enough different ids around, I just want to make the code clear).

@@ +337,5 @@
>    },
>  
>    onClick: function (event) {
>      if (this.download.succeeded) {
> +      this.download.launch().catch(Cu.reportError);

So this catch will never fire if I understand you? Or maybe it will not fire in all the same cases as below. I don't think we care about the smae things here, so that's probably fine...

::: mobile/android/modules/DownloadNotifications.jsm
@@ +64,5 @@
>        this._viewAdded = false;
>      }
>    },
>  
> +  getDownloadForNotification: function (id) {

Do we need to pull this out of known ids now?

@@ +111,5 @@
>    this._fileName = OS.Path.basename(download.target.path);
>  
>    this.id = null;
> +  // Known IDs are notification IDs that have been passed to about:downloads via onClick()
> +  this.knownIds = new Set();

Are there more than one id's for a particular notification. That feels like we've messed up somewhere if its happening....

@@ +210,5 @@
> +  showInAboutDownloads: function (id) {
> +    // remember the ID so that about:downloads can fetch the related download
> +    this.knownIds.add(id);
> +
> +    browserApp.addTab("about:downloads?notification-id=" + id);

Hmm.. Lets be a little more formal here and use something like BrowserDOMWindow. For example:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsWebHandlerApp.js#104

Comment 19

[James Gilbertson]

(In reply to Wesley Johnston (:wesj) from comment #18)
> @@ +337,5 @@
> >    },
> >  
> >    onClick: function (event) {
> >      if (this.download.succeeded) {
> > +      this.download.launch().catch(Cu.reportError);
> 
> So this catch will never fire if I understand you? Or maybe it will not fire
> in all the same cases as below. I don't think we care about the smae things
> here, so that's probably fine...

Right now, it'll never fire.

> ::: mobile/android/modules/DownloadNotifications.jsm
> @@ +64,5 @@
> >        this._viewAdded = false;
> >      }
> >    },
> >  
> > +  getDownloadForNotification: function (id) {
> 
> Do we need to pull this out of known ids now?

Sorry, not quite sure what you're asking here.

> @@ +111,5 @@
> >    this._fileName = OS.Path.basename(download.target.path);
> >  
> >    this.id = null;
> > +  // Known IDs are notification IDs that have been passed to about:downloads via onClick()
> > +  this.knownIds = new Set();
> 
> Are there more than one id's for a particular notification. That feels like
> we've messed up somewhere if its happening....

Notifications only have one ID. But if a notification is dismissed, and then recreated, it'll have a new one. I admit I don't see that happening in the ordinary case, but it's the same amount of code to track one ID, so I thought I might as well cover all the bases.

Comment 20

[Wesley Johnston (:wesj)]

(In reply to James Gilbertson from comment #19)
> Sorry, not quite sure what you're asking here.

I wondered if we needed to remove the id from the set somewhere? Unlikely to cause problems, but just seems like good behavior.

Comment 21

[James Gilbertson]

(In reply to Wesley Johnston (:wesj) from comment #20)
> I wondered if we needed to remove the id from the set somewhere? Unlikely to
> cause problems, but just seems like good behavior.

If the ID is removed when it's found, about:downloads won't be able to get the download if the user manually reloads the tab for some reason.

Other then that, there's no reason stopping us from removing the ID if it's found.

Comment 22

[Marco Castelluccio [:marco]]

Filed bug 1004495 for the other issue I was experiencing (I've noticed I can reproduce it on my phone too).

Comment 23

[James Gilbertson]

Attachment: Show download in about:downloads if notification fails to launch (v3)

I've stripped out notification ids.

The notification now just gets a tab for about:downloads, and then asks it to scroll the associated download into view.

Comment 24

[Wesley Johnston (:wesj)]

Comment on attachment 8416401 [details] [diff] [review]
Show download in about:downloads if notification fails to launch (v3)

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

::: mobile/android/chrome/content/aboutDownloads.js
@@ +391,5 @@
> +    return Promise.resolve(downloadLists);
> +  } else {
> +    return new Promise((resolve, reject) => {
> +      window.addEventListener("DownloadListsLoaded", {
> +        handleEvent: event => {

Can you just add a function here instead of this object?

::: mobile/android/modules/DownloadNotifications.jsm
@@ +212,5 @@
> +      }
> +    });
> +
> +    loaded.then(list => list.scrollDownloadIntoView(download))
> +          .catch(Cu.reportError);

TBH, I would rather pass this download in the hash for the url. i.e. about:downloads#someDownloadId. Reaching into the DOM like this feels hacky for me, and someone is likely to accidentally break it.. We'd have to register an onhashchange listener in the tab to handle url changes though.

Comment 25

[James Gilbertson]

(In reply to Wesley Johnston (:wesj) from comment #24)
> Comment on attachment 8416401 [details] [diff] [review]
> Show download in about:downloads if notification fails to launch (v3)
> 
> Review of attachment 8416401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/aboutDownloads.js
> @@ +391,5 @@
> > +    return Promise.resolve(downloadLists);
> > +  } else {
> > +    return new Promise((resolve, reject) => {
> > +      window.addEventListener("DownloadListsLoaded", {
> > +        handleEvent: event => {
> 
> Can you just add a function here instead of this object?

I used an object here so I could un-register the listener. If that's not required, sure, a function could be added instead.

> 
> ::: mobile/android/modules/DownloadNotifications.jsm
> @@ +212,5 @@
> > +      }
> > +    });
> > +
> > +    loaded.then(list => list.scrollDownloadIntoView(download))
> > +          .catch(Cu.reportError);
> 
> TBH, I would rather pass this download in the hash for the url. i.e.
> about:downloads#someDownloadId. Reaching into the DOM like this feels hacky
> for me, and someone is likely to accidentally break it.. We'd have to
> register an onhashchange listener in the tab to handle url changes though.

I'll switch back to using IDs.

However, I do feel like this is a a cleaner approach. There's less code, since there's no need to track IDs or look up download objects. Also, it's not doing a deep dive into the DOM: it's just calling a top level function (whenDownloadListLoaded), which handles the actual logic. It should be fairly hard to break; since presumably any refactoring would also update whenDownloadListLoaded.

Comment 26

[:Paolo Amadini]

For what it's worth, we do often call well-defined top-level functions on other windows on Desktop, and we want to move to a model without IDs (we still use some IDs for now). Also, identifiers would change after a restart, though in practice this is not an issue because, as far as I can tell, "about:" URIs are not reopened across sessions.

Comment 27

[James Gilbertson]

Attachment: Show download in about:downloads if notification fails to launch (v4)

4th try the charm?

Approach:
- notification attempts to launch download; if that fails:
- DownloadNotifications stores the download object for that notification as "selectedDownload"
- using that notification's ID as the URL hash, it opens a new tab or updates an existing about:downloads tab

- about:download checks location.hash when loading, if there is one, it fetches the selected download from DownloadNotification
- an event listener for onhashchange does the same

Note: the notifiction ID is just used to trigger the scrolling logic. It's not actually used to look up a download.

Comment 28

[Wesley Johnston (:wesj)]

Comment on attachment 8423625 [details] [diff] [review]
Show download in about:downloads if notification fails to launch (v4)

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

I accidentally backed Downloads.jsm out of Nightly builds last week, but since its out now, we're going to try and wait for some of these other fixes to be lined up before landing again (i.e. we want to remove any time pressure on this). I think I'll make a list of what I consider minimum-requirements and we can try to get through them together or with other contributors.

That said, this looks pretty good to me. I think ideally I'd rather pass the output file path rather than an id hashcode. That gives something more persistent. But for the most part users shouldn't see these urls at all anyway, so persistence is kinda silly to worry about. I'm fine with this as is. Maybe you can file a follow up to use the path instead?

We'll land this when we reland the patch from bug 901360.

::: mobile/android/chrome/content/browser.js
@@ +1021,4 @@
>      } else {
>        this.selectTab(tab);
>      }
> +    return tab;

Technically, you don't need this change anymore. I'm fine with leaving it if you want.

::: mobile/android/modules/DownloadNotifications.jsm
@@ +206,5 @@
> +    for (let tab of window.BrowserApp.tabs) {
> +      if (tab.browser.currentURI.spec.startsWith("about:downloads")) {
> +        window.BrowserApp.selectTab(tab);
> +        tab.window.location.hash = hash;
> +        return;

Argh! Sorry to make your life awful. But nice fix :)

Comment 29

[Aaron Train [:aaronmt]]

Tapping a download notification is currently broken, it restarts Firefox even when the browser is not running: bug 1075476.

Comment 30

[Aaron Train [:aaronmt]]

Not sure which bug we want open. In entirety, download notifications are busted.

Comment 31

[:Margaret Leibovic]

This should be fixed with the patches that landed in bug 901360.

Aaron, can you verify the new patches in bug 901360 didn't cause a regression here? If things are looking good, we can just close this out.