Closed Bug 1113092 Opened 6 years ago Closed 6 years ago

Android download notifications are displayed every time Firefox is opened

Categories

(Firefox for Android :: Download Manager, defect)

37 Branch
ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 37
Tracking Status
firefox34 --- unaffected
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 --- verified
fennec 37+ ---

People

(Reporter: u421692, Assigned: Margaret)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached file 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.
Sounds similar to bug 1113110
tracking-fennec: --- → ?
I've checked, and the files are not downloaded, only the notifications are displayed.
Duplicate of this bug: 1113110
This may be a dupe of bug 1111659.
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 37+
Obvious workaround, if you're happy losing the file: delete it from Tools > Downloads.
Duplicate of this bug: 1111659
Attachment #8540283 - Flags: review?(paolo.mozmail)
/r/1637 - Bug 1113092 - Don't create new download notifications for succeeded downloads

Pull down this commit:

hg pull review -r 4238be0e7aa7ca5e38da9a0b9b1b2f1bf9ac40fb
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.
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.
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 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.
Attachment #8540283 - Flags: review?(paolo.mozmail)
(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.
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).
Attachment #8540283 - Flags: review?(paolo.mozmail)
Attachment #8540283 - Flags: review?(mark.finkle)
/r/1637 - Bug 1113092 - Don't create new download notifications for succeeded downloads

Pull down this commit:

hg pull review -r 8f0338f5d3495259b77b1fbcea48497a9f59e025
I tested this new patch by killing and restarting the app with succeeded/paused/canceled downloads. Everything works as expected.
Attachment #8540283 - Flags: review?(mark.finkle) → review+
Attachment #8540283 - Flags: review?(paolo.mozmail) → review+
https://hg.mozilla.org/mozilla-central/rev/9143bca09fcc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Verified as fixed on latest Nightly on Nexus 7(Android 5.0.1)
Status: RESOLVED → VERIFIED
Attachment #8540283 - Attachment is obsolete: true
Attachment #8618935 - Flags: review+
You need to log in before you can comment on or make changes to this bug.