e-mail compose in SeaMonkey crashes [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsCommandManager::CommandStatusChanged ]

RESOLVED FIXED in seamonkey2.52

Status

P3
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: frg, Assigned: frg)

Tracking

(Blocks: 1 bug, {crash, regression})

SeaMonkey 2.47 Branch
seamonkey2.52
crash, regression
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.48 fixed, seamonkey2.49esr fixed, seamonkey2.50 wontfix, seamonkey2.51 fixed, seamonkey2.52 fixed)

Details

(crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
+++ This bug was initially created as a clone of Bug #1308923 +++

User agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 SeaMonkey/2.48a2
Build identifier: 20161006025928

Reproduction:
1. open new message compose windows with Ctrl+M

Actual:
New window appears and than SeaMonkey crashes.

https://crash-stats.mozilla.com/report/index/7c766d92-2115-49be-b8b3-855732161010

MOZ_CRASH Reason	ElementAt(aIndex = 2, aLength = 2)

I am creating a new bug for SeaMonkey only because it also happens in Thunderbird with different code in place.  The cause for this must be different.

In SeaMonkey it happens in:

https://dxr.mozilla.org/comm-central/source/suite/mailnews/compose/MsgComposeCommands.js

The obs_documentCreated observer is removed while observing said topic. This causes a crash in embedding/components/commandhandler/nsCommandManager.cpp because the observer array size is not supposed to change there. It does not happen on all systems.
(Assignee)

Comment 1

2 years ago
Created attachment 8867793 [details] [diff] [review]
1364977-messagecrash.patch

I am not so familiar with commandObservers. I am assuming that each editor has its own CommandManager and can have different observers. If this is not the case the patch might need changes. Tested with several windows and it worked. No errors reported in the console.
Attachment #8867793 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

2 years ago
Blocks: 1313304
(Assignee)

Updated

2 years ago
Blocks: 1351984
(Assignee)

Updated

2 years ago
Blocks: 1334779
(Assignee)

Updated

2 years ago
Blocks: 1345770

Comment 2

2 years ago
Comment on attachment 8867793 [details] [diff] [review]
1364977-messagecrash.patch

Is it worth revisiting the some of the suggestions from bug 468740?
(Assignee)

Comment 3

2 years ago
Created attachment 8869848 [details] [diff] [review]
1364977-messagecrash-V2.patch

Not removing the observer in the observer causes bug 468740 again. Per Neil in Bug 468740 Comment 4 using the openURL function from toolkit's contentAreaUtils.js could be used. I opted to take the OpenSelectedAttachment function from TBs OpenSelectedAttachment MsgComposeCommands.js which more or less does the same. Just Reformatted it.

Instead of the call to Services.io.newChannelFromURI2 this could be used:

> let channel = NetUtil.newChannel({
>   uri,
>   loadUsingSystemPrincipal: true
> });

But it uses the same exact call to newChannelFromURI2 internally and for clarity I actually prefer the TB code.
Attachment #8867793 - Attachment is obsolete: true
Attachment #8867793 - Flags: review?(iann_bugzilla)
Attachment #8869848 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

2 years ago
Blocks: 1346939

Updated

2 years ago
tracking-seamonkey2.48: --- → ?

Comment 4

2 years ago
Comment on attachment 8869848 [details] [diff] [review]
1364977-messagecrash-V2.patch

> function OpenSelectedAttachment()
> {
>+  let bucket = document.getElementById("attachmentBucket");
>+  if (bucket.selectedItems.length == 1) {
>+    let attachmentUrl = bucket.getSelectedItem(0).attachment.url;
>+
>+    let messagePrefix = /^mailbox-message:|^imap-message:|^news-message:/i;
>+    if (messagePrefix.test(attachmentUrl)) {
>+      // We must be dealing with a forwarded attachment, treat this special.
Nit: specially
>+      let msgHdr = gMessenger.messageServiceFromURI(attachmentUrl).messageURIToMsgHdr(attachmentUrl);
I'd prefer to stick with gMessenger.msgHdrFromURI(attachmentUrl) here.

r=me
Attachment #8869848 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 5

2 years ago
Created attachment 8874359 [details] [diff] [review]
1364977-messagecrash-V2a.patch

NIT fixed. First thought the different message header processing was a recent change but it seems to be so for ages. Only did go back to 1.9.1.

https://hg.mozilla.org/comm-central/rev/1694753e30a0ca0f1231f59bd63aee43bf5728e9

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: SeaMonkey might crash when trying to compose an email.
Testing completed (on m-c, etc.): comm-esr52 to comm-central
Risk to taking this patch (and alternatives if risky): low risk and no alternative.
String changes made by this patch: none

c-r approval is meant for 2.48
Attachment #8869848 - Attachment is obsolete: true
Attachment #8874359 - Flags: review+
Attachment #8874359 - Flags: approval-comm-release?
Attachment #8874359 - Flags: approval-comm-esr52?
Attachment #8874359 - Flags: approval-comm-beta?
(Assignee)

Updated

2 years ago
status-seamonkey2.50: affected → wontfix
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-seamonkey2.52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.52
Version: Trunk → SeaMonkey 2.47 Branch

Comment 6

2 years ago
Comment on attachment 8874359 [details] [diff] [review]
1364977-messagecrash-V2a.patch

a=me for the requested repos
Attachment #8874359 - Flags: approval-comm-release?
Attachment #8874359 - Flags: approval-comm-release+
Attachment #8874359 - Flags: approval-comm-esr52?
Attachment #8874359 - Flags: approval-comm-esr52+
Attachment #8874359 - Flags: approval-comm-beta?
Attachment #8874359 - Flags: approval-comm-beta+
(Assignee)

Updated

2 years ago
tracking-seamonkey2.48: ? → ---

Comment 9

2 years ago
So is this also needed in Thunderbird? Is bug 1308923 the TB version?

Comment 10

2 years ago
As FRG observed in bug 1308923 comment #28, we never remove the observer added with this call:
    GetCurrentCommandManager().
            addCommandObserver(gMsgEditorCreationObserver, "obs_documentCreated");

For good house-keeping we could remove it in ComposeUnload().

We already have the nsAttachmentOpener stuff from the SM patch.

Comment 11

2 years ago
Created attachment 8875483 [details] [diff] [review]
1364977.patch - TB patch for the same issue.
Attachment #8875483 - Flags: review?(acelists)

Comment 12

2 years ago
Created attachment 8875485 [details] [diff] [review]
1364977.patch - TB patch for the same issue.
Attachment #8875483 - Attachment is obsolete: true
Attachment #8875483 - Flags: review?(acelists)
Attachment #8875485 - Flags: review?(acelists)

Comment 13

2 years ago
Comment on attachment 8875485 [details] [diff] [review]
1364977.patch - TB patch for the same issue.

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

Thanks. So does this fix bug 1308923 or is that different?
Attachment #8875485 - Flags: review?(acelists) → review+
(Assignee)

Comment 14

2 years ago
aceman. Not sure but I think not. The problem here was that it was removed during notification processing so the array used in the calling code changed. That is also why I spun off this bug. There must be other causes for this but probably only add-on related.

Comment 15

2 years ago
Landed for Thunderbird, won't do uplifts:
https://hg.mozilla.org/comm-central/rev/16d84314fbf7d7b505aeee78674127f38dc1d1a9

Sorry for hijacking your bug ;-)
(Assignee)

Updated

2 years ago
No longer blocks: 1334779
(Assignee)

Updated

2 years ago
Blocks: 1334779
You need to log in before you can comment on or make changes to this bug.