1036186 - Reset Migration wizard no longer skips the first step to choose a browser

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[Marco Mucci [:MarcoM]]

+++ This bug was initially created as a clone of Bug #1032958 +++

Comment 1

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

Mano, what's the state of this bug? Should it be added to iteration 34.1?

Comment 2

[:Gijs (out for now; he/him)]

m-c pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c8288d0c7a15&tochange=62d33e3ba514

June 6 - june 7, which per calendar is exactly in time for the 32 cutoff :'( . I'll doublecheck with beta in a second. I'll also continue bisecting because nothing in that backlog really jumps out at me...

Comment 3

[:Gijs (out for now; he/him)]

Just realized that maybe the affected state and so on needs to be in the blocking bug? It's not really clear :-\

Comment 4

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> m-c pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=c8288d0c7a15&tochange=62d33e3ba514
> 
> June 6 - june 7, which per calendar is exactly in time for the 32 cutoff :'(
> . I'll doublecheck with beta in a second. I'll also continue bisecting
> because nothing in that backlog really jumps out at me...

This was wrong, sadly...

This should be better: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cbe4f69c2e9c&tochange=e017c15325ae

Comment 5

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from bug 1032958 comment #5)
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=cbe4f69c2e9c&tochange=e017c15325ae
> 
> there's several things in here... an asyncshutdown change and the window
> webidl switch...

For the window webidl switch (which was one of my guesses), I would look at the following first:

(In reply to Matthew N. [:MattN] from bug 1032958 comment #4)
> https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/
> content/migration.js?rev=875df781fa8b&mark=34-39,101-105#34

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Matthew N. [:MattN] from comment #5)
> (In reply to :Gijs Kruitbosch from bug 1032958 comment #5)
> > http://hg.mozilla.org/mozilla-central/
> > pushloghtml?fromchange=cbe4f69c2e9c&tochange=e017c15325ae
> > 
> > there's several things in here... an asyncshutdown change and the window
> > webidl switch...
> 
> For the window webidl switch (which was one of my guesses), I would look at
> the following first:
> 
> (In reply to Matthew N. [:MattN] from bug 1032958 comment #4)
> > https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/
> > content/migration.js?rev=875df781fa8b&mark=34-39,101-105#34

Yes. Further narrowed down to http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ad154c4c3111&tochange=448f2153d6d3 which corresponds to the window webidl switch. I'll look at this next.

Comment 7

[:Gijs (out for now; he/him)]

So this is definitely because of bug 789261, confirmed by checking both the build with it and the build immediately previous to it.

It seems that setting the selectedItem of the radiogroup here:

https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/content/migration.js?rev=875df781fa8b&mark=90#90

breaks with an exception that NodeFilter is undefined in radio.xml here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/radio.xml?rev=f41a267983c1&mark=281#281

I checked, and it is also undefined in the caller in migration.js, so this doesn't seem to be a chrome vs. XBL visibility thing.

The function in migration.js that makes this call isn't run until onload ( see http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/content/migration.xul#14 ) and so I'm a little lost as to why NodeFilter isn't defined at that point yet. I was under the impression that it should be... By the time that the user clicks a button in the dialog, the same code runs and it is defined fine.

I'm also not sure what any of this has to do with the webidl-ifying of the window object... Peter, can you help me out? :-)

Comment 8

[Peter Van der Beken [:peterv]]

What are the steps to reproduce?

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Peter Van der Beken [:peterv] from comment #8)
> What are the steps to reproduce?

1. Launch trunk/nightly with the profile manager
2. Create a profile
3. select the profile from the profile manager and start Firefox with it
4. Open "about:support"
5. Click "Reset Firefox/Nightly" on the top right
6. In the dialog that pops up, indicate you really really want to reset Firefox

Now Firefox will restart and automatically open the migration wizard.

The expected result is that we want it to bypass the "Where would you like to import data from" step (which will give you options based on the other browsers on the machine). Since bug 789261 landed, that step of the wizard is no longer being bypassed. The details of the technical side of the "why doesn't it work" there, inasmuch as I managed to figure them out, are in comment #7.

Comment 10

[Peter Van der Beken [:peterv]]

Hmm, at the point were this code runs we haven't called mozilla::dom::Register. We probably used to call this through NS_GetDOMClassInfoInstance when wrapping the window object, but that doesn't happen anymore with Window on WebIDL.

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: v1

Before we moved Window to WebIDL the creation of the XPConnect wrapper for the first window object would end up calling nsDOMClassInfo::Init through NS_GetDOMClassInfoInstance. That in turn would register all the DOM class names in the script namespace manager. Now that Window is on WebIDL we don't call NS_GetDOMClassInfoInstance for it and so it's possible that nsDOMClassInfo::Init isn't called until way after we've started interacting with the window object from JS, and the DOM class names are missing from the window object. The patch makes us call nsDOMClassInfo::Init at startup instead (some of the changes are to avoid recursively initializing the layout module).

Comment 12

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05e4c47f1e98

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for Mulet mochitest bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c54d3d2983

https://tbpl.mozilla.org/php/getParsedLog.php?id=45544560&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45542304&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45543177&tree=Mozilla-Inbound

Comment 14

[Peter Van der Beken [:peterv]]

I've been debugging this on try, since I can't reproduce it locally for some reason. The problem is that we load the layout module during component registration, because we try to create a script error for the double registration of a CID. At that point the component for mozApps hasn't been registered yet and nsScriptNameSpaceManager::OperateCategoryEntryHash tries to map the contract ID to a CID. We'll need to somehow delay the registering of DOM names to after all component registration is done.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v2

The additional bool check in CreateGlobal is a little sad, but the alternative is adding an xpcom-startup observer, which I dislike more.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8471847 [details] [diff] [review]
v2

Why you don't just use sRegisteredDOMNames inside RegisterDOMNames() ?
RegisterDOMNames() could return NS_OK, if it was successfully called once.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 18

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

Is this realistically upliftable to Beta 32?

Comment 19

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e349895861e2

(In reply to :Gavin Sharp [email: [e-mail]] from comment #18)
> Is this realistically upliftable to Beta 32?

I think so, yes.

Comment 20

[Ed Morley [:emorley]]

Reverted for crashing:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45932317&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45932238&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/909de22e9dec

Comment 21

[Peter Van der Beken [:peterv]]

Grmbl, we can't register the names when initing worker globals.

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: v2.2

PostCreateGlobal is called from CreateGlobal, right after creating the JS object. It should be fine to delay the registering of the names until then, and it allows us to do this only when wrapping Window objects, all other globals use different mechanisms to define the names anyway.

https://tbpl.mozilla.org/?tree=Try&rev=b92791987dcf

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

If we're going to take this on beta (and I think that is an if at this point in the schedule), it's going to need to land on m-c on Tue, Aug 19, so that it can bake for at least a couple of days before beta9 goes to build on Thu.

Comment 24

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8addf953e8eb

Comment 25

[Peter Van der Beken [:peterv]]

Comment on attachment 8473244 [details] [diff] [review]
v2.2

Approval Request Comment
[Feature/regressing bug #]: regressed by bug 789261
[User impact if declined]: "Reset Firefox/Nightly" doesn't work as intended, causing user confusion because a dialog shows up that shouldn't
[Describe test coverage new/current, TBPL]: don't think there's an automated test for the broken feature, :Gijs?
[Risks and why]: fairly low risk, the fix makes sure that part of the initialisation of the DOM happens even when using the new DOM bindings for Window. The backouts were primarily triggered by unusual conditions of how our tests are run.
[String/UUID change made/needed]: None

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8addf953e8eb

Comment 27

[:Gijs (out for now; he/him)]

(In reply to Peter Van der Beken [:peterv] from comment #25)
> [Describe test coverage new/current, TBPL]: don't think there's an automated
> test for the broken feature, :Gijs?

No, because it involves a browser restart and we don't really have a good framework to deal with those kinds of tests.

Comment 28

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/608831ad65bd

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm going to wait on the beta approval until tomorrow in the hope that we can get this verified on m-c or aurora before uplifting. Marking QA wanted and ni on juanb.

Comment 30

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

Worked fine in Nightly, 34.0a1 (2014-08-21) on MacOSX 10.8.5 and Windows 7. There wasn't a step to choose a browser, and the reset worked smoothly.

Comment 31

[juan becerra [:juanb]]

Clearing needinfo request thanks to Liz's testing. We will re-test on tonight's beta for completeness.

Comment 32

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8473244 [details] [diff] [review]
v2.2

Now that this has been verified on m-c, approving for beta.

Comment 33

[Peter Van der Beken [:peterv]]

Attachment: v2.2 merged to beta

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ac8864d8ecc0

Comment 35

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on: 
- Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.4 using latest Aurora 33.0a2 (buildID: 20140821004002)
- Ubuntu 13.10 32bit and Mac OSX 10.9.4 using Firefox 32 Beta 9 (buildID: 20140822024446)

Comment 36

[Tracy Walker [:tracy]]

This looks good on Windows with Fx32beta9