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

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, defect, P3)

Description

[Frank-Rainer Grahl (:frg)]

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

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 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.

Comment 2

[Ian Neal]

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

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

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: 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.

Comment 4

[Ian Neal]

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

Comment 5

[Frank-Rainer Grahl (:frg)]

Attachment: 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

Comment 6

[Ian Neal]

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

a=me for the requested repos

Comment 7

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-beta/rev/12f117e6fe7f0a6fff1b1ae23e7e4145d9c8304d
https://hg.mozilla.org/releases/comm-esr52/rev/1b1eaf39219815e3b1a4e83244d1bb2917931e89

2.48 release branch:
https://hg.mozilla.org/releases/comm-release/rev/9f4f453bf5bc557422d7f25bc48522f1eb1ad5c3

Comment 8

[Frank-Rainer Grahl (:frg)]

Drat I goofed. Forgot that the patch needed 2 small changes for 2.48 and 2.49:

https://hg.mozilla.org/releases/comm-esr52/rev/7f2073ee1fa4dc0fdf75f65d0228cdfeafc7bba1
https://hg.mozilla.org/releases/comm-release/rev/156fd1a6486a07df761b712c01621a37716f36b2

Comment 9

[:aceman]

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

Comment 10

[Jorg K (CEST = GMT+2)]

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

[Jorg K (CEST = GMT+2)]

Attachment: 1364977.patch - TB patch for the same issue.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1364977.patch - TB patch for the same issue.

Comment 13

[:aceman]

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?

Comment 14

[Frank-Rainer Grahl (:frg)]

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

[Jorg K (CEST = GMT+2)]

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

Sorry for hijacking your bug ;-)