Closed
Bug 1036186
Opened 10 years ago
Closed 10 years ago
Reset Migration wizard no longer skips the first step to choose a browser
Categories
(Firefox :: Migration, defect)
Tracking
()
VERIFIED
FIXED
Firefox 34
People
(Reporter: MarcoM, Assigned: peterv)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
smaug
:
review+
Sylvestre
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1032958 +++
Flags: firefox-backlog+
Reporter | ||
Updated•10 years ago
|
QA Whiteboard: [qa-]
Updated•10 years ago
|
Assignee: nobody → mano
Comment 1•10 years ago
|
||
Mano, what's the state of this bug? Should it be added to iteration 34.1?
Flags: needinfo?(mano)
Reporter | ||
Updated•10 years ago
|
Assignee: mano → nobody
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 34.2
Flags: needinfo?(mano)
Comment 2•10 years ago
|
||
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...
status-firefox33:
--- → affected
status-firefox34:
--- → affected
Comment 3•10 years ago
|
||
Just realized that maybe the affected state and so on needs to be in the blocking bug? It's not really clear :-\
status-firefox33:
affected → ---
status-firefox34:
affected → ---
Comment 4•10 years ago
|
||
(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•10 years ago
|
||
(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•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
What are the steps to reproduce?
Flags: needinfo?(peterv) → needinfo?(gijskruitbosch+bugs)
Comment 9•10 years ago
|
||
(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•10 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•10 years ago
|
||
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)
Updated•10 years ago
|
Iteration: 34.2 → ---
Points: 2 → ---
QA Whiteboard: [qa-] → [qa+]
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
tracking-firefox32:
--- → +
tracking-firefox33:
--- → +
tracking-firefox34:
--- → +
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
Updated•10 years ago
|
Attachment #8469318 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Comment 13•10 years ago
|
||
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
Assignee | ||
Comment 14•10 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•10 years ago
|
||
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 16•10 years ago
|
||
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•10 years ago
|
||
Attachment #8471847 -
Attachment is obsolete: true
Attachment #8472225 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8472225 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 19•10 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)
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Grmbl, we can't register the names when initing worker globals.
Assignee | ||
Comment 22•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8473244 -
Flags: review?(bugs) → review+
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
Assignee | ||
Comment 25•10 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)
Comment 26•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•10 years ago
|
Attachment #8473244 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 27•10 years ago
|
||
(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)
Assignee | ||
Comment 28•10 years ago
|
||
Comment 29•10 years ago
|
||
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.
Comment 31•10 years ago
|
||
Clearing needinfo request thanks to Liz's testing. We will re-test on tonight's beta for completeness.
Updated•10 years ago
|
Flags: needinfo?(jbecerra)
Comment 32•10 years ago
|
||
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+
Assignee | ||
Comment 33•10 years ago
|
||
Comment 34•10 years ago
|
||
Comment 35•10 years ago
|
||
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)
Updated•10 years ago
|
Comment 36•10 years ago
|
||
This looks good on Windows with Fx32beta9
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•