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

VERIFIED FIXED in Firefox 32

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: MarcoM, Assigned: peterv)

Tracking

32 Branch
Firefox 34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32+ verified, firefox33+ verified, firefox34+ verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

5 years ago
+++ This bug was initially created as a clone of Bug #1032958 +++
Flags: firefox-backlog+
Reporter

Updated

5 years ago
QA Whiteboard: [qa-]
Mano, what's the state of this bug? Should it be added to iteration 34.1?
Flags: needinfo?(mano)
Reporter

Updated

5 years ago
Assignee: mano → nobody
Reporter

Updated

5 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 34.2
Flags: needinfo?(mano)
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...
Just realized that maybe the affected state and so on needs to be in the blocking bug? It's not really clear :-\
(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
(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
(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.
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? :-)
Flags: needinfo?(peterv)
Assignee

Comment 8

5 years ago
What are the steps to reproduce?
Flags: needinfo?(peterv) → needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(peterv)
Assignee

Comment 10

5 years ago
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.
Assignee: gijskruitbosch+bugs → peterv
Flags: needinfo?(peterv)
Assignee

Comment 11

5 years ago
Posted patch v1 (obsolete) — Splinter Review
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).
Attachment #8469318 - Flags: review?(bugs)
Blocks: 789261
No longer blocks: 1032958
Iteration: 34.2 → ---
Points: 2 → ---
QA Whiteboard: [qa-] → [qa+]
Depends on: 1032958
Flags: firefox-backlog+
Summary: Investigation: Reset Migration wizard no longer skips the first step to choose a browser → Reset Migration wizard no longer skips the first step to choose a browser
Attachment #8469318 - Flags: review?(bugs) → review+
Assignee

Comment 14

5 years ago
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.
Assignee

Comment 15

5 years ago
Posted patch v2 (obsolete) — Splinter Review
The additional bool check in CreateGlobal is a little sad, but the alternative is adding an xpcom-startup observer, which I dislike more.
Attachment #8469318 - Attachment is obsolete: true
Attachment #8471847 - Flags: review?(bugs)
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.
Attachment #8471847 - Flags: review?(bugs) → review-
Assignee

Comment 17

5 years ago
Posted patch v2.1 (obsolete) — Splinter Review
Attachment #8471847 - Attachment is obsolete: true
Attachment #8472225 - Flags: review?(bugs)
Attachment #8472225 - Flags: review?(bugs) → review+
Is this realistically upliftable to Beta 32?
Flags: needinfo?(peterv)
Assignee

Comment 19

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

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #18)
> Is this realistically upliftable to Beta 32?

I think so, yes.
Flags: needinfo?(peterv)
Assignee

Comment 21

5 years ago
Grmbl, we can't register the names when initing worker globals.
Assignee

Comment 22

5 years ago
Posted patch v2.2Splinter Review
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
Attachment #8472225 - Attachment is obsolete: true
Attachment #8473244 - Flags: review?(bugs)
Attachment #8473244 - Flags: review?(bugs) → review+
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.
Assignee

Comment 25

5 years ago
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
Attachment #8473244 - Flags: approval-mozilla-beta?
Attachment #8473244 - Flags: approval-mozilla-aurora?
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/8addf953e8eb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Attachment #8473244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(jbecerra)
Keywords: qawanted
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.
Clearing needinfo request thanks to Liz's testing. We will re-test on tonight's beta for completeness.
Flags: needinfo?(jbecerra)
Comment on attachment 8473244 [details] [diff] [review]
v2.2

Now that this has been verified on m-c, approving for beta.
Attachment #8473244 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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)
This looks good on Windows with Fx32beta9
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.