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

VERIFIED FIXED in Firefox 43

Status

()

Firefox
Migration
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Trunk
Firefox 45
Points:
---

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ verified, firefox44+ verified, firefox45+ verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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... :-\
(Assignee)

Comment 1

2 years ago
Created 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
Attachment #8680579 - Flags: review?(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.
(Assignee)

Comment 3

2 years ago
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)
(Assignee)

Comment 4

2 years ago
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.
Attachment #8680579 - Flags: review?(jaws) → review+
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?
(Assignee)

Comment 6

2 years ago
(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.
(Assignee)

Comment 7

2 years ago
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?
(Assignee)

Updated

2 years ago
tracking-firefox43: --- → ?
https://reviewboard.mozilla.org/r/23643/#review21167

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

unused variable
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 9

2 years ago
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
(Assignee)

Comment 10

2 years ago
(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.
tracking-firefox43: ? → +
tracking-firefox44: --- → +
Flags: needinfo?(wkocher)
Keywords: checkin-needed
Tracking since this is a big recent regression and we want to make sure it's fixed asap.
status-firefox45: --- → affected
tracking-firefox45: --- → +
status-firefox42: --- → unaffected

Comment 13

2 years ago
(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.

Comment 15

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/1300e56c184d
Keywords: checkin-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.
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
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(Assignee)

Comment 19

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

beta is still closed.
status-firefox44: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/25cea6767faa
status-firefox43: affected → fixed
(Assignee)

Comment 21

2 years ago
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
Flags: qe-verify+
QA Contact: vasilica.mihasca

Comment 22

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1300e56c184d
status-b2g-v2.5: --- → fixed
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: --- → ---

Comment 24

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/91a7bd126142
status-b2g-v2.5: --- → fixed

Comment 25

2 years ago
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!
status-b2g-v2.5: fixed → ---
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 26

2 years ago
(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)

Comment 27

2 years ago
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
status-firefox43: fixed → verified
status-firefox44: fixed → verified
status-firefox45: fixed → verified
Flags: qe-verify+

Comment 28

2 years ago
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.