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

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P4)

Description

[Carsten Book [:Tomcat]]

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]

Comment 2

[Mike Connor [:mconnor]]

this doesn't cause anything user-facing, and its in the context of the window we're destroying, no?

Comment 3

[Marcus Redivo]

Attachment: Additionally check whether sourceProfiles is null

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.

Comment 4

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

Which migrator implementation returns null for sourceProfiles?

Comment 5

[Marcus Redivo]

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.

Comment 6

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

(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?

Comment 7

[Marcus Redivo]

Attachment: Set this._migrator = null when "Don't import anything" selected

"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.

Comment 8

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

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.

Comment 9

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

Marcus: can you take this bug?

Comment 10

[Marcus Redivo]

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?

Comment 11

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

(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.

Comment 12

[Marcus Redivo]

Attachment: Avoid advancing to next page when not necessary

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.

Comment 13

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

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;".

Comment 14

[Marcus Redivo]

(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.

Comment 15

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

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 ".

Comment 16

[Marcus Redivo]

Attachment: Return FALSE to avoid advancing to next page when not necessary

Gavin, thanks for the tip.

Comment 17

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

Comment on attachment 309696 [details] [diff] [review]
Return FALSE to avoid advancing to next page when not necessary

Thanks for the patch! :)

Comment 18

[Reed Loden [:reed]]

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