1113092 - Android download notifications are displayed every time Firefox is opened

Status: VERIFIED FIXED

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

Description

[u421692]

Attachment: download_logs

Build: Nightly 37.0a1 (2014-12-18)

Steps to reproduce:
1. Open Firefox 
2. Download some files (e.g. ftp://ftp.mozilla.org/pub/mobile/)
3. Close Firefox
4. Open Firefox

Expected result:
Firefox is opened

Actual result:
Firefox is opened, and the download toast notification is displayed, and android download notifications are displayed again for the files downloaded in the last session.

Comment 1

[Aaron Train [:aaronmt]]

Sounds similar to bug 1113110

Comment 2

[u421692]

I've checked, and the files are not downloaded, only the notifications are displayed.

Comment 4

[:Margaret Leibovic]

This may be a dupe of bug 1111659.

Comment 5

[Richard Newman [:rnewman]]

Obvious workaround, if you're happy losing the file: delete it from Tools > Downloads.

Comment 7

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1113092/margaret

Comment 8

[:Margaret Leibovic]

/r/1637 - Bug 1113092 - Don't create new download notifications for succeeded downloads

Pull down this commit:

hg pull review -r 4238be0e7aa7ca5e38da9a0b9b1b2f1bf9ac40fb

Comment 9

[:Margaret Leibovic]

I found that we still showed the "Download started..." toast notifications if the download was canceled, so I added a check to avoid that.

I looked through the comments in bug 901360 to try to remember what was wrong with this approach, but I'm having trouble understanding how to update it to deal with the potential race issue of a new download succeeding before onDownloadAdded is called.

Paolo, do you have an example of how this could happen? I tried reproducing with very small downloads on a fast wifi network, but I didn't see any problems.

Comment 10

[Richard Newman [:rnewman]]

It happened to me with a download that I had successfully completed the previous day.

* Download a PDF.
* View it.
* Go on with my life (probably including using the browser!).
* Overnight, browser likely evicted from memory.
* Open a link in the morning. Notification appears.

So STR are likely to be:

* Complete a download.
* Kill the package with `am`.
* Relaunch the activity.

Comment 11

[:Margaret Leibovic]

Sorry if I wasn't clear, the thing I couldn't reproduce is a notification *not* being added for a succeeded download with this patch applied. I was speaking about an issue raised in bug 901360 comment 65.

The bug as described here is indeed very easy to reproduce by killing Fennec.

Comment 12

[:Paolo Amadini]

Comment on attachment 8540283 [details]
MozReview Request: bz://1113092/margaret

(In reply to :Margaret Leibovic from comment #9)
> Paolo, do you have an example of how this could happen? I tried reproducing
> with very small downloads on a fast wifi network, but I didn't see any
> problems.

It might not be reproducible with the current code. We have enough promise resolution cycles in the saver, so that "succeeded" is always set after the add notification. PDF saving code may have less cycles, but they're probably still enough.

But have you tried setting a boolean flag after list.addView(this) resolves? Maybe you can reuse _viewAdded, note how you can call removeView unconditionally in "uninit" because calling it twice does not cause an error. You may need a separate boolean for other synchronous initialization, though.

You can then try and check it in addition to the "succeeded" call, like I suggested in bug 901360 comment 72.

Also, the currentBytes check does not seem robust to me - the toast should probably be shown for new downloads when the boolean flag is set, that means never during startup.

Comment 13

[:Margaret Leibovic]

(In reply to :Paolo Amadini from comment #12)
> Comment on attachment 8540283 [details]
> MozReview Request: bz://1113092/margaret
> 
> (In reply to :Margaret Leibovic from comment #9)
> > Paolo, do you have an example of how this could happen? I tried reproducing
> > with very small downloads on a fast wifi network, but I didn't see any
> > problems.
> 
> It might not be reproducible with the current code. We have enough promise
> resolution cycles in the saver, so that "succeeded" is always set after the
> add notification. PDF saving code may have less cycles, but they're probably
> still enough.
> 
> But have you tried setting a boolean flag after list.addView(this) resolves?
> Maybe you can reuse _viewAdded, note how you can call removeView
> unconditionally in "uninit" because calling it twice does not cause an
> error. You may need a separate boolean for other synchronous initialization,
> though.
>
> You can then try and check it in addition to the "succeeded" call, like I
> suggested in bug 901360 comment 72.

I tried using the current this._viewAdded boolean flag, and it wasn't working as expected, but I now realize that's because that we don't wait for the promise to resolve before setting that to true. I don't know why we even need the flag the way it's currently used, since we only call init() once during startup.

I'll try setting a boolean flag when the promise resolves as you suggest, and hopefully that will solve my issue.
 
> Also, the currentBytes check does not seem robust to me - the toast should
> probably be shown for new downloads when the boolean flag is set, that means
> never during startup.

Yeah, I agree. I can do more to fix that.

Comment 14

[:Margaret Leibovic]

Also, we never call DownloadNotifications.uninit, so we can just get rid of that. (On Android we decided to just get rid of shutdown handlers, since the majority of the time "shutdown" happens when we are killed by the OS, so we don't have time to call these handlers).

Comment 15

[:Margaret Leibovic]

/r/1637 - Bug 1113092 - Don't create new download notifications for succeeded downloads

Pull down this commit:

hg pull review -r 8f0338f5d3495259b77b1fbcea48497a9f59e025

Comment 16

[:Margaret Leibovic]

I tested this new patch by killing and restarting the app with succeeded/paused/canceled downloads. Everything works as expected.

Comment 17

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

https://reviewboard.mozilla.org/r/1635/#review1069

Comment 18

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/1635/#review1071

Ship It!

Comment 19

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/9143bca09fcc

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9143bca09fcc

Comment 21

[u421692]

Verified as fixed on latest Nightly on Nexus 7(Android 5.0.1)

Comment 23

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1113092 - Don't create new download notifications for succeeded downloads