1280628 - update badge shown after fallback to complete update (Tor 19411)

Status: RESOLVED WORKSFORME

(Firefox :: Toolbars and Customization, defect, P3)

Description

[Mark Smith [:mcs]]

If a partial update fails to apply and the update falls back to downloading a full update, the update badge is displayed when it should not be (with the corresponding "Restart to Update" text inside the hamburger menu).

Steps to reproduce:
1. Download and install a Firefox that is slightly out of date.
2. Artificially modify a file that is usually updated between releases so that a partial update is very likely to fail. I re-zipped the top-level omni.ja file.
3. Ensure that app.update.badge == true.
4. Trigger an update check, e.g., open "About Firefox"

Expected Result:
No badge will be shown until the update is properly staged and the browser is ready for restart.

Actual Result:
The badge is displayed as soon as the partial update fails (while the complete MAR file is still being downloaded).

Here is the corresponding Tor Browser ticket:
https://trac.torproject.org/projects/tor/ticket/19411

Comment 1

[Mark Smith [:mcs]]

Attachment: proposed fix

Here is the fix we applied to Tor Browser. There may also be other cases that this code does not handle correctly.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8763231 [details] [diff] [review]
proposed fix

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

Not sure if this shouldn't just check for anything not in a particular list of known-good states. I dug around in the updater code for a bit but it was hairy (seems we generally send two observer notifications in short succession for which we both listen?) so I think it's likely best if :rstrong or someone else more familiar with the updater code reviews this.

Comment 3

[Robert Strong (they/them - no direct email)]

Stephen, I tried to reproduce on Windows without success... could you check why the observer was notified on your Mac? It doesn't look like it should be and it doesn't appear to be on Windows. You should be able to just log the parameters here
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2631

with
Services.console.logStringMessage("*** topic: " + topic + ", status: " + status);

Then change app.update.url.override to the update url for the previous days build, change app.update.staging to false, and try to update.

Comment 4

[Robert Strong (they/them - no direct email)]

I assumed that this happened on startup when it could be for the staging phase. It would be good to check with app.update.staging set to true as well.

Comment 5

[Robert Strong (they/them - no direct email)]

I think I was able to reproduce this when staging.

Comment 6

[Robert Strong (they/them - no direct email)]

Mark, can you verify whether this was seen before or after restarting? Thanks!

Comment 7

[Mark Smith [:mcs]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #6)
> Mark, can you verify whether this was seen before or after restarting?

Before restarting. More precisely: "The badge is displayed as soon as the partial update fails (while the complete MAR file is still being downloaded)."

Comment 8

[Robert Strong (they/them - no direct email)]

Thanks for the info Mark. I'm going to run a few tests locally before reviewing this and I had some other work take priority. I hope to have this done by Friday.

Comment 9

[Georg Koppen]

Robert, is there anything you still need from us? We really like to get this into ESR 52.

Comment 10

[Robert Strong (they/them - no direct email)]

I've had a couple of other things I've had to finish up and the staging code has been changed recently so this needs to be checked again. If I'm not able to get this landed in time for the nightly to aurora merge I'll request that it is uplifted.

Comment 11

[Georg Koppen]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)
> I've had a couple of other things I've had to finish up and the staging code
> has been changed recently so this needs to be checked again. If I'm not able
> to get this landed in time for the nightly to aurora merge I'll request that
> it is uplifted.

Thanks. Where are we with that one right now? Anything we can help with given that even the aurora merge window is closing soon?

Comment 12

[Mark Smith [:mcs]]

I would like rstrong to confirm, but my reading of the code that implements the doorhanger-based UI tells me that this bug has been fixed. Updating the badge is now done in a different way by new code. Also, I could not reproduce the original problem using a Firefox 60-based browser.

Comment 13

[Georg Koppen]

Robert: I guess the answer to my questions in comment:11 is that we are done here? Can you confirm that?

Comment 14

[Robert Strong (they/them - no direct email)]

Definitely should be working now.