If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Bug 1101100 broke nsWebBrowserPersist onStateChange callbacks in some error cases.

RESOLVED FIXED in Firefox 42

Status

()

Firefox
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

({regression})

42 Branch
Firefox 44
regression
Points:
---

Firefox Tracking Flags

(firefox42+ fixed, firefox43+ fixed, firefox44+ fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
The first two parts of nsWebBrowserPersist::EndDownload seem to be backwards, such that if the call to EndDownload is the first thing to report an error, then onStateChange gets NS_OK.

This doesn't affect onStatusChange, which might be why the download list GUI still shows “Failed”.  I'm a little fuzzy on what this actually *does* mean in terms of the front-end, but it seems like a pretty obvious bug, and the patch should be trivial, and it if this is going to go into 42 then that should happen soon.
(Assignee)

Comment 1

2 years ago
Created attachment 8667074 [details] [diff] [review]
bug1207864-wbp-onstate-hg0.diff

Bill: I hate to give you even more reviews, but this patch is small and you probably know this code better than anyone besides me at this point.

Paolo: Do you know how important the |status| argument of the onStateChange callback (not the onStatusChange callback) is for the front-end?  I'm trying to figure out if this is important enough to request uplift.
Attachment #8667074 - Flags: review?(wmccloskey)
Attachment #8667074 - Flags: feedback?(paolo.mozmail)

Comment 2

2 years ago
(In reply to Jed Davis [:jld] from comment #1)
> Paolo: Do you know how important the |status| argument of the onStateChange
> callback (not the onStatusChange callback) is for the front-end?  I'm trying
> to figure out if this is important enough to request uplift.

It's quite important that we don't receive a success code with onStateChange when there are the STATE_STOP and STATE_IS_NETWORK flags, if the download actually failed.

With regard to onStatusChange, we only care if we received a failure code through it, before the onStateChange notification with the STATE_STOP and STATE_IS_NETWORK flags occurred.

See here for the code that listens to these notifications:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadLegacy.js#83

Updated

2 years ago
Attachment #8667074 - Flags: feedback?(paolo.mozmail) → feedback+
Attachment #8667074 - Flags: review?(wmccloskey) → review+

Comment 3

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/44c1e75e7266
(Assignee)

Comment 4

2 years ago
(Note to self to request uplift, based on comment #2, once this has been on Nightly for a bit.)
Flags: needinfo?(jld)
https://hg.mozilla.org/mozilla-central/rev/44c1e75e7266
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(Assignee)

Comment 6

2 years ago
Comment on attachment 8667074 [details] [diff] [review]
bug1207864-wbp-onstate-hg0.diff

Approval Request Comment
[Feature/regressing bug #]: Bug 1101100
[User impact if declined]: We're not reporting errors from “Save as… Complete Document” correctly to the frontend; I'm a little unclear on what exactly this means for the user, but comment #2 says this is important.
[Describe test coverage new/current, TreeHerder]: Not as much as I'd like. There are mochitests that make sure nsWebBrowserPersist's basic functionality works, at least.  And this has been on m-c for a few days without problems.
[Risks and why]: None, as far as I can tell — the patch is very simple, and just restores the previous behavior.
[String/UUID change made/needed]: None.
Flags: needinfo?(jld)
Attachment #8667074 - Flags: approval-mozilla-beta?
Attachment #8667074 - Flags: approval-mozilla-aurora?
Since e10s is off in beta, we may not need to uplift this to 42. Tracking for 43 though.
status-firefox42: --- → affected
status-firefox43: --- → affected
tracking-firefox43: --- → +
tracking-firefox44: --- → +
Paolo, is this really broken in 42 right now?
Flags: needinfo?(paolo.mozmail)
Jed, can you make sure this is really broken in 42? I'm asking because the regressing bug seems to have something to do with e10s, which is disabled in the beta channel.
tracking-firefox42: --- → ?
Flags: needinfo?(jld)
Keywords: regression
(Assignee)

Comment 10

2 years ago
I'm pretty sure it's broken in non-e10s mode, but I'll do a build to double-check.
(Assignee)

Comment 11

2 years ago
Confirmed; I modified my testcase from bug 1204626 to check whether the expected errors are reported via onStateChange, and it fails in non-e10s mode on beta without this patch (and on m-c with the patch reverted).
Flags: needinfo?(jld)
OK, thanks very much Jed!
Tracking for 42 as well, then.
tracking-firefox42: ? → +
Comment on attachment 8667074 [details] [diff] [review]
bug1207864-wbp-onstate-hg0.diff

Fix for download list UI regression, looks ok on m-c.
Approved for uplift to aurora and beta.
Attachment #8667074 - Flags: approval-mozilla-beta?
Attachment #8667074 - Flags: approval-mozilla-beta+
Attachment #8667074 - Flags: approval-mozilla-aurora?
Attachment #8667074 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/457cd535b48b
status-firefox43: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/170d90101b72
status-firefox42: affected → fixed

Updated

2 years ago
Flags: needinfo?(paolo.mozmail)
You need to log in before you can comment on or make changes to this bug.