Closed Bug 413275 Opened 13 years ago Closed 13 years ago
Script Error: "source Profiles is null when Migration Wizard is canceled
Flags: blocking-firefox3? → blocking-firefox3+
this doesn't cause anything user-facing, and its in the context of the window we're destroying, no?
Priority: -- → P4
This error happens in the case where the user selects "Don't import anything" and clicks "Next", in which case this._migrator is non-null, but this._migrator.sourceProfiles *is* null.
Which migrator implementation returns null for sourceProfiles?
Not sure we can pick any particular one to blame; after all, the user selected "Don't import anything". I have not seen this error if a source profile has been selected.
(In reply to comment #5) > Not sure we can pick any particular one to blame; after all, the user selected > "Don't import anything". Right, I meant: what does this._migrator refer to after the user picked "Don't import anything"? Shouldn't it be null?
"this._migrator" turns out to be the migrator passed in when the dialog was launched. There was no code to clear it when the user selected "Don't import anything". This patch takes care of that, and consequently prevents the JS error. An alternate fix would abort the process entirely, instead of advancing to profile selection. There is one more point. From nsIBrowserProfileMigrator.idl: /** * An enumeration of available profiles. If the import source does * not support profiles, this attribute is null. */ readonly attribute nsISupportsArray sourceProfiles; I'm not certain if this page gets bypassed when the import source does not support profiles, but if it doesn't, there should still be a check for NULL. I left this check in.
It looks like the real problem here is that onSelectProfilePageShow is being called at all. Since onImportSourcePageAdvanced cancels the dialog we shouldn't end up in onSelectProfilePageShow at all, but dialog.xml's advance() looks for a specific return value of "false" from the pageadvanced handler to avoid progressing further (and ultimately firing the next page's "pageshow" event). So I think we should fix this by having onImportSourcePageAdvanced return "false" rather than undefined. We can also take the null check of sourceProfiles, I suppose, but the nsProfileMigrator code that launches this dialog will only pass in a migrator if the migrator's GetSourceExists returns true (or if it's IE on windows, which always has a "profile"), so in practice it won't be a problem.
Marcus: can you take this bug?
Yes, I can take this bug. I can't find the "Take bug" checkbox or the "Assignee" textbox, neither here nor when adding an attachment; insufficient permissions, I presume?
(In reply to comment #10 > I can't find the "Take bug" checkbox or the "Assignee" textbox, neither here > nor when adding an attachment; insufficient permissions, I presume? Ah, yeah. I've granted you editbugs/canconfirm, you should be able to do it now.
When the user selects "Don't import anything", we need to clear the 'next="selectProfile"' property defined in migration.xul. By doing so, the wizard does not erroneously execute code for the next page when canceling. The "next" property is updated later in the function, so it would appear that it could be left out of the XUL. That's not a good idea; the "Next" button then becomes the "Finish" button. It also appears that the "selectProfile" page is only selected to be shown when the source is known to support multiple profiles, so guarding access to this._migrator.sourceProfiles (as in the previous patches) is not necessary.
Does the fix I suggest in the last sentence of the second paragraph of comment 8 not work? That is, change: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/migration/content/migration.js&rev=1.35&mark=161#153 to be "return false;" rather than "return;".
(In reply to comment #13) > Does the fix I suggest in the last sentence of the second paragraph of comment > 8 not work? > > That is, change: > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/migration/content/migration.js&rev=1.35&mark=161#153 > > to be "return false;" rather than "return;". > Nope, doesn't work. document.documentElement.cancel(); actually calls window.close, but the handler for the next page still executes. Inspection also shows that the return value does not make it back to here: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/wizard.xml#427 where the value of "returned" is "undefined" both as the code currently is, and when returning false.
Oh, you probably also need to change: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/migration/content/migration.xul&rev=1.28&mark=59#57 and add a "return ".
Gavin, thanks for the tip.
Comment on attachment 309696 [details] [diff] [review] Return FALSE to avoid advancing to next page when not necessary Thanks for the patch! :)
Attachment #309696 - Flags: review? → review+
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → Firefox 3 beta5
Checking in browser/components/migration/content/migration.js; /cvsroot/mozilla/browser/components/migration/content/migration.js,v <-- migration.js new revision: 1.36; previous revision: 1.35 done Checking in browser/components/migration/content/migration.xul; /cvsroot/mozilla/browser/components/migration/content/migration.xul,v <-- migration.xul new revision: 1.29; previous revision: 1.28 done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.