Closed Bug 413275 Opened 13 years ago Closed 13 years ago

JavaScript Error: "sourceProfiles is null when Migration Wizard is canceled

Categories

(Firefox :: Migration, defect, P4)

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta5

People

(Reporter: cbook, Assigned: mredivo)

References

Details

Attachments

(1 file, 3 obsolete files)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012014 Firefox/3.0b3pre

Steps to reproduce:
- Rename or delete old profile folders
- Start Firefox
- Migration Wizard comes up
- Cancel the Migration Wizard Dialog
- JS Error is shown in Debug Build output

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "sourceProfiles is null" {file: "chrome://browser/content/migration/migration.js" line: 177}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
Flags: blocking-firefox3?
Duplicate of this bug: 415981
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.
Attachment #307115 - Flags: review?
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.
Attachment #308454 - Flags: review?(gavin.sharp)
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?
Attachment #308454 - Attachment is obsolete: true
Attachment #308454 - Flags: review?(gavin.sharp)
Attachment #307115 - Attachment is obsolete: true
Attachment #307115 - Flags: review?
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.
Assignee: nobody → mredivo
Status: NEW → ASSIGNED
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.
Attachment #309534 - Flags: review?
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.
Gavin, thanks for the tip.
Attachment #309534 - Attachment is obsolete: true
Attachment #309534 - Flags: review?
Attachment #309696 - Flags: review?
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+
Keywords: checkin-needed
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
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.