Closed Bug 1186141 Opened 9 years ago Closed 9 years ago

Still Outlook import crash via Tools | Import in Thunderbird 38.1.0, after bug 1175055

Categories

(MailNews Core :: Import, defect)

x86
All
defect
Not set
critical

Tracking

(thunderbird40 wontfix, thunderbird41 fixed, thunderbird42 fixed, thunderbird43 fixed, thunderbird_esr3840+ fixed)

RESOLVED FIXED
Thunderbird 43.0
Tracking Status
thunderbird40 --- wontfix
thunderbird41 --- fixed
thunderbird42 --- fixed
thunderbird43 --- fixed
thunderbird_esr38 40+ fixed

People

(Reporter: wsmwk, Assigned: jorgk-bmo)

References

Details

(Keywords: crash, topcrash-thunderbird)

Crash Data

Attachments

(2 files, 1 obsolete file)

I thought the Outlook migration at initial install of TB was disabled by bug 1175055.
I won't have access to the machine with Outlook I used for testing until 13th Aug. 2015.
I am certainly fielding questions in support about it not working and being disabled.  So we must have disabled it.
The outlook crashes are currently ranked #9 and #10 for 38.1.0, so still very highly ranked indeed. The feedback I am getting by private email from users is they are using Tools > Import. So there must be a code path that is still enabled.

https://crash-stats.mozilla.com/search/?product=Thunderbird&version=38.1.0&date=%3E2015-07-01&user_comments=~import&_facets=signature&_columns=date&_columns=signature&_columns=version&_columns=build_id&_columns=user_comments#crash-reports
Flags: needinfo?(rkent)
Flags: needinfo?(mozilla)
Summary: Outlook import crash → Still Outlook import crash via Tools | Import in Thunderbird 38.1.0, after bug 1175055
As I said in comment #1: I can't work on this until 13th Aug. 2015.
Do these users use "Import Everything" or do they import specific items?
As far as I can see on my travel laptop, all Outlook and Eudora import options are disabled.
We could speed this up if we had a screenshot of what they are doing.
Several users reported they used "Import Everything" which offered outlook. One of those observed that the outlook choice was "selected" but was also "disabled". I'm not sure what that means. 

Attempting to get clarifications from users.
Yes, exactly!!

Outlook is offered, selected (if it's the only choice) and disabled (greyed out). From there, it should be *impossible* to click "Next", since no valid option was selected.
Code:
#ifdef XP_WIN
 <radio id="oexpress"  label="&importFromOExpress.label;"  accesskey="&importFromOExpress.accesskey;"/>
 <radio id="outlook"   label="&importFromOutlook.label;"   accesskey="&importFromOutlook.accesskey;"
                       tooltiptext="Currently disabled due to bug 1175055" disabled="true"/>
 #endif

I won't repeat when I can look at it again.
Flags: needinfo?(mozilla)
Although the option is disabled, you can still press "Next".
I could swear I tested that in bug 1175055, as far as I remember you couldn't press "Next". I'll look into it.
You can land this on TB 38.x to fix the crashes.

Note to the reviewer:
This bit:
+    if (!this._migrator) {
+      return;
+    }
has nothing to do with the fix. I noticed a JS error (dereferenced null) on "Import Nothing", so I fixed it.

If one day we fix the Outlook and Eudora import, this patch does *NOT* have to be rolled back. The only one that needs to be rolled back is in bug 1175055.
Flags: needinfo?(rkent)
Attachment #8645387 - Flags: review?(rkent)
Assignee: nobody → mozilla
I don't know how much testing reviewers do. You can test this patch without Outlook and Eudora.

Just install SeaMonkey on your machine, then edit 
  comm-central/mail/components/migration/content/migration.xul
and add
  tooltiptext="Currently disabled due to bug 1175055" disabled="true"
to the SeaMonkey item.
"I don't know how much testing reviewers do. You can test this patch without Outlook and Eudora."

Even with Outlook, the import code AFAICT only supports Outlook 98 and Outlook 2003 - and I don't have those old Outlooks! Hard to get too, now that Technet is discontinued so there is no affordable official source of Microsoft test software.

Anyway I did my own test (by changing the registry key location to point to the Outlook that I do have installed) and can confirm that I can generate a crash without this patch, but the crash is prevented (because the "Outlook" entry though found is disabled with the patch, but enabled without) and the "Next" is disabled with this patch, and enabled without.
Comment on attachment 8645387 [details] [diff] [review]
Fix up wizard to deal with disabled items. Don't select them and don't allow "Next" with no valid selection.

Review of attachment 8645387 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with suggested revisions. I'll also post the patch that I tested that include those revisions, you can land that patch if you also agree with the revisions.

::: mail/components/migration/content/migration.js
@@ +94,5 @@
>      }
>      group.selectedItem = this._source == "" ? firstNonDisabled : document.getElementById(this._source);
> +
> +    if (!firstNonDisabled) {
> +      var nextButton = this._wiz.getButton("next");

Generally the "next" button is controlled by the wizard, and it is not good to look into the wizard and change its UI elements. That's what canAdvance is for, so please use that.

I also prefer that canAdvance be disabled at the top of this, and only enabled when we have firstNonDisabled so that we default to do nothing. Better if there are unknown errors.
Attachment #8645387 - Flags: review?(rkent) → review+
I think we should take this for 38.2.0
Comment on attachment 8645387 [details] [diff] [review]
Fix up wizard to deal with disabled items. Don't select them and don't allow "Next" with no valid selection.

I was looking for a solution that involved the wizard controls, but couldn't figure it out. Kent's solution works and is much cleaner.
Attachment #8645387 - Attachment is obsolete: true
Comment on attachment 8646080 [details] [diff] [review]
With RKJ revision

Review of attachment 8646080 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works! Thanks for improving on my initial suggestion.
Attachment #8646080 - Flags: review+
Comment on attachment 8646080 [details] [diff] [review]
With RKJ revision

[Approval Request Comment]
User impact if declined: Outlook import enabled and crashing.
Testing completed (on c-c, etc.): Yes, both Jorg and Kent tested in different ways.
Risk to taking this patch (and alternatives if risky):
Small wizard tweak, no risk.
Attachment #8646080 - Flags: approval-comm-esr38?
Attachment #8646080 - Flags: approval-comm-beta?
Attachment #8646080 - Flags: approval-comm-aurora?
http://hg.mozilla.org/comm-central/rev/974d4f960cf2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 8646080 [details] [diff] [review]
With RKJ revision

https://hg.mozilla.org/releases/comm-esr38/rev/9653ee53a6fa
Attachment #8646080 - Flags: approval-comm-esr38? → approval-comm-esr38+
Comment on attachment 8646080 [details] [diff] [review]
With RKJ revision

http://hg.mozilla.org/releases/comm-aurora/rev/446ee8397c77
https://hg.mozilla.org/releases/comm-beta/rev/4f1d6e940fc4
Attachment #8646080 - Flags: approval-comm-beta?
Attachment #8646080 - Flags: approval-comm-beta+
Attachment #8646080 - Flags: approval-comm-aurora?
Attachment #8646080 - Flags: approval-comm-aurora+
Target Milestone: --- → Thunderbird 43.0
Blocks: 432902
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: