497578 - Fallback upgrade ends in endless loop of downloading partial upgrades when Private Browsing auto-start is enabled

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: updater log

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4 ID:20090423191946

While we were sitting on update testing last week for b99 I was noticing that for one of my profiles the fallback method doesn't work. Each time Firefox claimed that the update could not be applied but didn't wanted to download the full update. Instead I ended in an infinite loop in downloading partial updates.

The following steps can be made to reproduce the loop on all platforms. The mentioned profile I will upload immediately.

Steps:
1. Start Firefox 3.5b4 with the given profile
2. Check for new updates
3. Download partial updates and don't restart Firefox
4. Close Firefox manually
5. Remove the first characters from the downloaded updates.mar file
6. Start Firefox again
7. Notice the warning dialog that the update could not be applied because an unkown reason.
8. There is no Next button to start the full download
9. Check for new updates
10. Start the download

After step 10 you will see that Firefox is downloading the partial update again. You can repeat those steps as often as you want. It's an endless loop. The following messages were added by the application updater. See the attached picture.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: affected profile

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I reduced the profile as much as I had time for the last hour. I hope it's enough to see why we are failing.

Comment 3

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

Henrik, could you verify that setting the pref
user_pref("browser.privatebrowsing.autostart", true);
causes this? Thanks

Comment 4

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

Double checked on a new profile with setting browser.privatebrowsing.autostart to true and I was able to reproduce this bug. :(

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

tbqh, I would settle for a relnote here, if I thought anyone would read them!

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Robert for the remaining investigation. This pref was an accidentally left-over from bug verifications of PB bugs. It's strange that this can cause such a trouble. Lets update the summary.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #5)
> tbqh, I would settle for a relnote here, if I thought anyone would read them!

What's the chance that people will hit this situation? Do we know how often partial updates fail?

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Does it need to download a corrupt partial, or would any network failure during the partial DL cause this?

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Partials fail for a bunch of reasons, but not all that often. Don't think this blocks, but would obviously like a fix, and to understand what we're blowing away in private browsing that causes this to happen.

Comment 10

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

A corrupted download is handled by the file hash check so the case where I see that this would happen is when a file to be updated has been changed and the crc check then failed.

Comment 11

[juan becerra [:juanb]]

From talking to the Numerator and build and release people, the ratio of partials:completes might be around 2:1, for the last few versions. Which is sort of a big number of completes and could indicate a rather large number of partial failures. We're in the middle of 3.0.11 release, but there it is for your consideration.

Comment 12

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

Also note that if a person is not running the latest version their next update will be a complete

Comment 13

[juan becerra [:juanb]]

My original question to the Numerator was: what is the number of partial vs complete updates for the latest version prior update offer. 2:1 seems too many completes, but we could sift through the data we have.

Comment 14

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

Per mconnor when entering private browsing necko is reset which is causing the XHR to return NS_ERROR_NOT_INITIALIZED in onStopRequest. With the browser.privatebrowsing.autostart pref added by bug 460346 set to true necko is reset on startup while the XHR is being made by app update. The app update XHR request can be moved to final-ui-startup (the same point session restore uses to avoid similar issues) to fix this bug and it is my understanding that private browsing will be fixing the necko reset at some point in the future. Patch coming up.

Comment 15

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

Attachment: patch rev1

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 382863 [details] [diff] [review]
patch rev1

r=me, but the comment should just list the items that we'll do on final-ui-startup, the current wording is a little strange

Comment 17

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

Attachment: patch with nit fixed

Carrying forward review

Comment 18

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

Attachment: patch rev3

Some cruft snuck in to the change to the test head file

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 382885 [details] [diff] [review]
patch rev3

a191=beltzner

Rob tells me he might have an alternate fix, but this seems pretty straightforward and solid to me. Seems to have passed unit tests on tryserver, too.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/f9a4e14e1678
https://hg.mozilla.org/releases/mozilla-1.9.1/rev/190289693897

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't think that we can have an automated test for this problem, or? If it's not possible shall we create at least a Litmus test? I know that this is very specific but it would give us the chance to cover regressions. We don't have a test for fallback upgrades until now.

Comment 22

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

(In reply to comment #19)
> (From update of attachment 382885 [details] [diff] [review])
> a191=beltzner
> 
> Rob tells me he might have an alternate fix, but this seems pretty
> straightforward and solid to me. Seems to have passed unit tests on tryserver,
> too.
I'm more comfortable with the current fix than the alternative at present. I'll look at the alternative after the 3.5 release since it is cleaner though more invasive.

Gavin, thanks for the checkin!

Henrik, thanks for catching this... I'm looking into adding a test but it won't be finished soon so a litmus test would be a good thing if it isn't too much trouble.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

(removing relnote keyword)

Rob: can you come up with a testing matrix that we can use to hammer away on this during the QA period?

Comment 24

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

(In reply to comment #23)
> (removing relnote keyword)
> 
> Rob: can you come up with a testing matrix that we can use to hammer away on
> this during the QA period?
Will do

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 with builds on Windows and OS X:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090613 Minefield/3.6a1pre ID:20090613032901

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090613 Shiretoko/3.5pre ID:20090613031011

Robert, that was a really quick fix. Thanks you too.

Comment 26

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

Attachment: tests in progress

Comment 27

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

Attachment: test plus a minor change to nsUpdateService.js.in

Comment 28

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

Attachment: patch - tests

I'll file a separate bug for the minor change to nsUpdateService.js.in

Comment 29

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

(In reply to comment #28)
> Created an attachment (id=384464) [details]
> patch - tests
> 
> I'll file a separate bug for the minor change to nsUpdateService.js.in
Filed bug 499770

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Robert, seeing your work on the test makes me wonder if we still should add a litmus test for it. When do you think the test will be ready?

Comment 31

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

Hopefully soon so you might as well hold off

Comment 32

[Dave Townsend [:mossop]]

Why is the xpcom-shutdown notification commented out in end_test?

Comment 33

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

It was cruft that snuck in while I was working on bug 499770

Comment 34

[Dave Townsend [:mossop]]

Comment on attachment 384464 [details] [diff] [review]
patch - tests

I guess you don't need that or the hunk in head_update.js with the other patch landed.

Comment 35

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

exactly

Comment 36

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

Attachment: test patch as checked in

Comment 37

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

Test patch pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/07fb0b9396fa

Comment 38

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

Attachment: 1.9.1 branch test patch as checked in

Comment 39

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

Test patch pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8d1d8901c213

Comment 40

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

Now that bug 496335 is fixed this workaround is going to be removed in bug 512651