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

RESOLVED FIXED in Thunderbird 43.0

Status

MailNews Core
Import
--
critical
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: wsmwk, Assigned: Jorg K (GMT+2))

Tracking

({crash, topcrash-thunderbird})

Thunderbird 43.0
x86
All
crash, topcrash-thunderbird
Dependency tree / graph

Thunderbird Tracking Flags

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

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #1175055 +++
+++ This bug was initially created as a clone of Bug #917961 +++

Some crashes are still occuring after bug 1175055, because signature is #5 crash for version 38.1.0
https://crash-stats.mozilla.com/report/list?product=Thunderbird&range_value=7&range_unit=days&date=2015-07-21&signature=mozilla%3A%3Adom%3A%3Aworkers%3A%3AGetCurrentThreadJSContext%28%29&version=Thunderbird%3A38.1.0#tab-comments

a couple examples:
bp-80cb63b8-8104-4ea9-83ed-6a3632150719
bp-08c5ca60-c59d-4476-ae66-da8ac2150714
(Reporter)

Updated

2 years ago
status-thunderbird_esr38: --- → affected
tracking-thunderbird_esr38: --- → ?
(Assignee)

Comment 1

2 years ago
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.

Comment 2

2 years ago
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)
(Reporter)

Updated

2 years ago
Summary: Outlook import crash → Still Outlook import crash via Tools | Import in Thunderbird 38.1.0, after bug 1175055
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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)
(Assignee)

Comment 7

2 years ago
Created attachment 8645370 [details]
Screenshot showing the problem.

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

Comment 8

2 years ago
Created 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.

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)

Updated

2 years ago
Assignee: nobody → mozilla
(Assignee)

Comment 9

2 years ago
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.

Comment 10

2 years ago
"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 11

2 years ago
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+

Comment 12

2 years ago
Created attachment 8646080 [details] [diff] [review]
With RKJ revision

Comment 13

2 years ago
I think we should take this for 38.2.0
tracking-thunderbird_esr38: ? → 40+
(Assignee)

Comment 14

2 years ago
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
(Assignee)

Comment 15

2 years ago
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+
(Assignee)

Comment 16

2 years ago
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?

Comment 17

2 years ago
http://hg.mozilla.org/comm-central/rev/974d4f960cf2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Comment 18

2 years ago
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+

Updated

2 years ago
status-thunderbird_esr38: affected → fixed

Comment 19

2 years ago
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+

Updated

2 years ago
status-thunderbird40: --- → wontfix
status-thunderbird41: --- → fixed
status-thunderbird42: --- → fixed
status-thunderbird43: --- → fixed
Target Milestone: --- → Thunderbird 43.0

Updated

11 months ago
Blocks: 432902
You need to log in before you can comment on or make changes to this bug.