Closed Bug 497578 Opened 12 years ago Closed 12 years ago

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

Categories

(Toolkit :: Application Update, defect)

1.9.1 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: whimboo, Assigned: robert.strong.bugs)

References

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(5 files, 5 obsolete files)

Attached image 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.
Flags: blocking1.9.2?
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.
Keywords: testcase
Summary: Fallback upgrade ends in endless loop of downloading of partial patch for given profile → Fallback upgrade ends in endless loop of downloading partial upgrades for given profile
Henrik, could you verify that setting the pref
user_pref("browser.privatebrowsing.autostart", true);
causes this? Thanks
Flags: blocking1.9.1?
Double checked on a new profile with setting browser.privatebrowsing.autostart to true and I was able to reproduce this bug. :(
tbqh, I would settle for a relnote here, if I thought anyone would read them!
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.
Summary: Fallback upgrade ends in endless loop of downloading partial upgrades for given profile → Fallback upgrade ends in endless loop of downloading partial upgrades when Private Browsing auto-start is enabled
(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?
Does it need to download a corrupt partial, or would any network failure during the partial DL cause this?
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.
Flags: wanted1.9.1.x+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Keywords: relnote
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.
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.
Also note that if a person is not running the latest version their next update will be a complete
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.
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.
Attached patch patch rev1 (obsolete) — Splinter Review
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #382863 - Flags: review?(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
Attachment #382863 - Flags: review?(mconnor) → review+
Attached patch patch with nit fixed (obsolete) — Splinter Review
Carrying forward review
Attachment #382863 - Attachment is obsolete: true
Attachment #382879 - Flags: review+
Attachment #382879 - Flags: approval1.9.1?
Attachment #382879 - Attachment is obsolete: true
Attachment #382879 - Flags: approval1.9.1?
Attached patch patch rev3Splinter Review
Some cruft snuck in to the change to the test head file
Attachment #382885 - Flags: review+
Attachment #382885 - Flags: approval1.9.1?
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.
Attachment #382885 - Flags: approval1.9.1? → approval1.9.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.
Target Milestone: --- → mozilla1.9.2a1
(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.
(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?
Keywords: relnote
(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
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
Flags: blocking1.9.2?
Attached patch tests in progress (obsolete) — Splinter Review
Attachment #384044 - Flags: review?(dtownsend)
Attached patch patch - tests (obsolete) — Splinter Review
I'll file a separate bug for the minor change to nsUpdateService.js.in
Attachment #384044 - Attachment is obsolete: true
Attachment #384464 - Flags: review?(dtownsend)
Attachment #384044 - Flags: review?(dtownsend)
(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
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?
Hopefully soon so you might as well hold off
Why is the xpcom-shutdown notification commented out in end_test?
It was cruft that snuck in while I was working on bug 499770
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.
Attachment #384464 - Flags: review?(dtownsend) → review+
Attachment #384464 - Attachment is obsolete: true
Attachment #384690 - Flags: review+
Test patch pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/07fb0b9396fa
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Now that bug 496335 is fixed this workaround is going to be removed in bug 512651
You need to log in before you can comment on or make changes to this bug.