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.

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.

(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

(Note to self to request uplift, based on comment #2, once this has been on Nightly for a bit.)

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.

Since e10s is off in beta, we may not need to uplift this to 42. Tracking for 43 though.

Paolo, is this really broken in 42 right now?

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.

I'm pretty sure it's broken in non-e10s mode, but I'll do a build to double-check.

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).

OK, thanks very much Jed! Tracking for 42 as well, then.

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.