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

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

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... :-\

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1219707 - fix argument passing to migration.js, r=jaws

Bug 1219707 - fix argument passing to migration.js, r?jaws

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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 3

[:Gijs (he/him)]

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

Comment 4

[:Gijs (he/him)]

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 5

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 6

[:Gijs (he/him)]

(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 7

[:Gijs (he/him)]

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

Comment 8

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

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

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

unused variable

Comment 9

[:Gijs (he/him)]

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

Comment 10

[:Gijs (he/him)]

(In reply to :Gavin Sharp [email: [e-mail]] 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!

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a big recent regression and we want to make sure it's fixed asap.

Comment 13

[Chris More [:cmore]]

(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

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1300e56c184d

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8680579 [details]
MozReview Request: Bug 1219707 - fix argument passing to migration.js, r=jaws

Approved

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1300e56c184d

Comment 19

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/91a7bd126142

beta is still closed.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/25cea6767faa

Comment 21

[:Gijs (he/him)]

This is now working properly on nightly \o/ :

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2015-10-31&keys=__none__!__none__!__none__&max_channel_version=nightly%252F45&measure=FX_MIGRATION_ENTRY_POINT&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-10-31&table=0&trim=1&use_submission_date=0

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1300e56c184d

Comment 23

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/91a7bd126142

Comment 25

[Ada [:adalucinet]]

Attachment: 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!

Comment 26

[:Gijs (he/him)]

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

Comment 27

[Ada [:adalucinet]]

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

Comment 28

[Ada [:adalucinet]]

Sorry for the above typo - it's 45.0a1, not 44.0a1.