Closed Bug 1219707 Opened 4 years ago Closed 4 years ago

bug 731025 broke passing arguments to migration.xul, messing up Firefox reset and migration telemetry results

Categories

(Firefox :: Migration, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 45
Tracking Status
firefox42 --- unaffected
firefox43 + verified
firefox44 + verified
firefox45 + verified

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(2 files)

In bug 731025 I started using plain JS arrays to be passed to nsIWindowWatcher's openWindow method. It turns out that just doesn't work. I'm not really sure why it seemed to work in my testing, but here we are... :-\
Bug 1219707 - fix argument passing to migration.js, r?jaws
Attachment #8680579 - Flags: review?(jaws)
Comment on attachment 8680579 [details]
MozReview Request: Bug 1219707 - fix argument passing to migration.js, r=jaws

https://reviewboard.mozilla.org/r/23643/#review21153

::: browser/components/migration/MigrationUtils.jsm:585
(Diff revision 1)
> +    // nsIWindowWatcher doesn't deal with raw arrays, so we convert the input

Your conversion here only supports booleans, integers (not sure what happens to floating point numbers that are stored in a PRUint32), and strings. These could all be supported much simpler by converting the array to JSON instead.
Comment on attachment 8680579 [details]
MozReview Request: Bug 1219707 - fix argument passing to migration.js, r=jaws

Re-requesting per discussion on IRC - we pass XPCOM objects as well, which this conversion also deals with. (Otherwise I agree converting to JSON and back would have been simpler.)
Attachment #8680579 - Flags: review?(jaws)
As for floats, it shouldn't be necessary to pass those here. It might help to look at https://hg.mozilla.org/mozilla-central/rev/681e438f6fcc#l1.67 to see how this worked before bug 731025.
Comment on attachment 8680579 [details]
MozReview Request: Bug 1219707 - fix argument passing to migration.js, r=jaws

https://reviewboard.mozilla.org/r/23643/#review21157

After speaking over IRC, Gijs pointed out that I missed a line in the conversion method that also accepts XPCOM arguments. What do we have in the way of tests for this, especially since this is fixing a regression?
(In reply to (Away 10/30 - 11/3) Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8680579 [details]
> MozReview Request: Bug 1219707 - fix argument passing to migration.js, r?jaws
> 
> https://reviewboard.mozilla.org/r/23643/#review21157
> 
> After speaking over IRC, Gijs pointed out that I missed a line in the
> conversion method that also accepts XPCOM arguments. What do we have in the
> way of tests for this, especially since this is fixing a regression?

Nada, largely because bug 888624, which didn't get fixed because it needed bug 940954.

(before bug 731025, the fx reset path was the only one that passed meaningful arguments)

I'll look again at fixing the tests situation after this lands.
Comment on attachment 8680579 [details]
MozReview Request: Bug 1219707 - fix argument passing to migration.js, r=jaws

Approval Request Comment
[Feature/regressing bug #]: bug 731025
[User impact if declined]: the --migration commandline flag, and because of that, the Firefox reset/refresh feature, does not work correctly. The user will get asked which browser and/or profile they want to import from (those parts of the wizard should be skipped in those situations).
[Describe test coverage new/current, TreeHerder]: none, see earlier comments.
[Risks and why]: well, no higher than bug 731025, which busted things here, and which said "medium-low". We all know how that turned out. I tested this change manually, and I am sure that this change makes a Firefox reset go back to working "normally", so from that perspective, "medium-low" again, I guess? :-\
[String/UUID change made/needed]: no
Attachment #8680579 - Flags: approval-mozilla-beta?
Attachment #8680579 - Flags: approval-mozilla-aurora?
https://reviewboard.mozilla.org/r/23643/#review21167

::: browser/components/migration/MigrationUtils.jsm:594
(Diff revision 1)
> +          let itemType = typeof item;

unused variable
Attachment #8680579 - Attachment description: MozReview Request: Bug 1219707 - fix argument passing to migration.js, r?jaws → MozReview Request: Bug 1219707 - fix argument passing to migration.js, r=jaws
Comment on attachment 8680579 [details]
MozReview Request: Bug 1219707 - fix argument passing to migration.js, r=jaws

Bug 1219707 - fix argument passing to migration.js, r=jaws
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #8)
> https://reviewboard.mozilla.org/r/23643/#review21167
> 
> ::: browser/components/migration/MigrationUtils.jsm:594
> (Diff revision 1)
> > +          let itemType = typeof item;
> 
> unused variable

Fixed, thanks!
From email from Gijs it sounds like we need to check this in and get it uplifted to 43 beta as soon as possible so we have it in place before we go to build for 43 beta 1.
Flags: needinfo?(wkocher)
Keywords: checkin-needed
Tracking since this is a big recent regression and we want to make sure it's fixed asap.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Tracking since this is a big recent regression and we want to make sure it's
> fixed asap.

+1
Wes is aware and will land this, but there is some problem with bustage right now (not from this) so it will take a little while.
Unless there's a really pressing need to get this to aurora/beta ASAP, I'd prefer to let this work its way from fx-team to m-c before uplifting. (It still needs a+'s, anyway.)

Assuming approvals happen in the next few hours, tomcat should be able to get this uplifted tonight.
Flags: needinfo?(wkocher)
Comment on attachment 8680579 [details]
MozReview Request: Bug 1219707 - fix argument passing to migration.js, r=jaws

Approved
Attachment #8680579 - Flags: approval-mozilla-beta?
Attachment #8680579 - Flags: approval-mozilla-beta+
Attachment #8680579 - Flags: approval-mozilla-aurora?
Attachment #8680579 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/1300e56c184d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Flags: qe-verify+
QA Contact: vasilica.mihasca
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Attached image Screenshot
Unable to see any difference between a build before the fix (42.0b9) and one after (43.0b2): the prompt where the user choose which browser and/or profile to import is no longer displayed after Refreshing Firefox, only the Import complete part (please see the attached screenshot).

Gijs, could you please add some STR in order to reproduce/verify this fix? Many thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #25)
> Created attachment 8685431 [details]
> Screenshot
> 
> Unable to see any difference between a build before the fix (42.0b9) and one
> after (43.0b2): the prompt where the user choose which browser and/or
> profile to import is no longer displayed after Refreshing Firefox, only the
> Import complete part (please see the attached screenshot).
> 
> Gijs, could you please add some STR in order to reproduce/verify this fix?
> Many thanks!

Your STR are fine, but you shouldn't compare 42.0b9, you should check a beta build before 43.0b1, see comment #11. You can find some on treeherder:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&fromchange=38f98cdc8d49&tochange=25cea6767faa

will have builds that include this fix at the top, and builds from the earlier revs will show the problem I described.
Flags: needinfo?(gijskruitbosch+bugs)
Successfully reproduced with this tinderbox build - http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win32/1446274610/
Verified fixed with 43.0b4 (Build ID: 20151116155110), latest 44.0a2 and 44.0a1, across platforms [1].

[1] Windows 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.8.5
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Sorry for the above typo - it's 45.0a1, not 44.0a1.
You need to log in before you can comment on or make changes to this bug.