Closed Bug 1470892 Opened 3 years ago Closed 3 years ago

New subscribe dialogue crashing @ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::RemoveElementsAt | nsSubscribableServer::ToggleOpenState via nsSubscribableServer::ToggleOpenState

Categories

(MailNews Core :: Backend, defect)

defect
Not set
critical

Tracking

(thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed, thunderbird63 fixed)

RESOLVED FIXED
Thunderbird 63.0
Tracking Status
thunderbird_esr60 --- fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed
thunderbird63 --- fixed

People

(Reporter: jorgk-bmo, Assigned: infofrommozilla)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 3 obsolete files)

Received this from Alfred Peters (whose BMO account was blocked for some obscure reasons):

I can reproducibly crash the TB with the new dialog.

bp-c5c8735f-a197-44ad-8a24-658fb0180625
bp-bb40cb58-0d0c-4797-8f2c-008cc0180617 (2018-06-17)

STR:

1. Open the dialog
2. Select the last sheet of the lowest branch
3. Type the left key so many times till the branch is closed
4. Type once the up key
5. Procede with 3.

After 2 to 5 closed branches TB will crash.

I used a news account. There exist more and deeper branches. The error also
occurs with an IMAP account.
Keywords: crashreportid
Both crashes are in
nsSubscribableServer::ToggleOpenState(int) - comm/mailnews/base/src/nsSubscribableServer.cpp:994

That's on Daily 62, so here:
https://searchfox.org/comm-central/rev/2ad95d3d40d07c90ff44d1fe9bada1243b3f8197/mailnews/base/src/nsSubscribableServer.cpp#994

Aceman, you know the logic better than me, can you please take a look.
Flags: needinfo?(acelists)
Severity: normal → critical
Crash Signature: [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::RemoveElementsAt | nsSubscribableServer::ToggleOpenState ]
Keywords: crashreportidcrash
Summary: New subscribe dialogue crashing → New subscribe dialogue crashing @ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::RemoveElementsAt | nsSubscribableServer::ToggleOpenState via nsSubscribableServer::ToggleOpenState
(In reply to Jorg K (GMT+2) from comment #1)
> That's on Daily 62, so here:
> <https://searchfox.org/comm-central/rev/2ad95d3d40d07c90ff44d1fe9bada1243b3f8197/mailnews/base/src/nsSubscribableServer.cpp#994>

#991| int32_t count = node->prevSibling
#992|   ? (mRowMap.IndexOf(node->prevSibling, aIndex) - aIndex - 1)
#993|   : (mRowMap.Length() - aIndex - 1);
#994| mRowMap.RemoveElementsAt(aIndex + 1, count);

The crash does occur when IndexOf() doesn't find node->prevSibling. Then IndexOf() gives back -1.
So count is negative. That leads to OutOfRange exception in RemoveElementsAt().

The $ 500 question is, why is the item not found?

The problem arises when a subtree is deleted but node->prevSibling is NULL.
Then count is (mRowMap.Length() - aIndex - 1). That deletes always the rest of the Array.
But it should only delete the nodes of the subtree.

count = (node->lastChild - node->firstChild +1)

should do the trick, but it's only correct in ~50% of the cases.
In ~49% (node->firstChild - node->lastChild +1) would be correct.
And sometimes both variants give a wrong result.
I suspect if the node name already appears in other branches.
The patch checks for ROOT and otherwise only deletes the subtree.
Attachment #8988980 - Flags: review?(jorgk)
Comment on attachment 8988980 [details] [diff] [review]
Fix crash by preventing a corrupt tree when closing a subtree in IMAP/NEWS subscribe dialog

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

Thanks for taking this on. Great that we can ship TB 60 ESR without this crash :-)

I think Aceman is more familiar with these data structures. I have a bunch of nits which we can fix before landing assuming Aceman gives r+.

::: mailnews/base/src/nsSubscribableServer.cpp
@@ +987,5 @@
>    if (node->isOpen)
>    {
>      // Close the node by finding the next sibling
>      node->isOpen = false;
> +    // Remove subtree -> delete elements till next sibling

I don't like the ->, let's just use English, ... and with a full stop at the end.

@@ +991,5 @@
> +    // Remove subtree -> delete elements till next sibling
> +    int32_t count = 0;
> +    do
> +    {
> +      // Do find next sibling or the beginning of the next subtree

No need to emphasise "*Do* find ...". Just "Find next ..." is OK, ... and with a full stop at the end.

@@ +992,5 @@
> +    int32_t count = 0;
> +    do
> +    {
> +      // Do find next sibling or the beginning of the next subtree
> +      if (node->prevSibling)

If the else branch uses braces, the if branch needs them, too, according to the style guide.

@@ +993,5 @@
> +    do
> +    {
> +      // Do find next sibling or the beginning of the next subtree
> +      if (node->prevSibling)
> +          count = mRowMap.IndexOf(node->prevSibling, aIndex) - aIndex - 1;

Indentation wrong here.
Attachment #8988980 - Flags: review?(jorgk)
Attachment #8988980 - Flags: review?(acelists)
Attachment #8988980 - Flags: feedback+
(In reply to Jorg K (GMT+2) from comment #4)
> Comment on attachment 8988980 [details] [diff] [review]

>   I have a bunch
> of nits which we can fix before landing

Ok - done.

This time I tried to follow the guideline.
I hope I found the right one. :-)
Attachment #8988980 - Attachment is obsolete: true
Attachment #8988980 - Flags: review?(acelists)
Attachment #8988994 - Flags: review?(acelists)
(In reply to Alfred Peters from comment #5)
> Created attachment 8988994 [details] [diff] [review]

Sorry, wrong patch file.

(In reply to Jorg K (GMT+2) from comment #4)
> Comment on attachment 8988980 [details] [diff] [review]

>   I have a bunch
> of nits which we can fix before landing

Ok - done.

This time I tried to follow the guideline.
I hope I found the right one. :-)
Attachment #8988994 - Attachment is obsolete: true
Attachment #8988994 - Flags: review?(acelists)
Attachment #8988995 - Flags: review?(acelists)
(In reply to Alfred Peters from comment #2)
> #991| int32_t count = node->prevSibling
> #992|   ? (mRowMap.IndexOf(node->prevSibling, aIndex) - aIndex - 1)
> #993|   : (mRowMap.Length() - aIndex - 1);
> #994| mRowMap.RemoveElementsAt(aIndex + 1, count);
> 
> The crash does occur when IndexOf() doesn't find node->prevSibling. Then
> IndexOf() gives back -1.
> So count is negative. That leads to OutOfRange exception in
> RemoveElementsAt().
> The $ 500 question is, why is the item not found?

And you also kept this same code in the patch, so when node->prevSibling is not null, IndexOf() gets called on it.
 
> The problem arises when a subtree is deleted but node->prevSibling is NULL.
> Then count is (mRowMap.Length() - aIndex - 1). That deletes always the rest
> of the Array.
> But it should only delete the nodes of the subtree.

Yes, I could now see this in the dialog. Collapsing a small branch removes all rows below it even from branches that are not children of it.

So the failing of IndexOf() seems to be not that current node's node->prevSibling can't be found via IndexOf, but some parent node's node->prevSibling isn't found in the array because we have incorrectly removed it from the array.
Flags: needinfo?(acelists)
Assignee: nobody → infofrommozilla
Blocks: 1466705
Status: NEW → ASSIGNED
Component: General → Backend
OS: Unspecified → All
Hardware: Unspecified → All
Comment on attachment 8988995 [details] [diff] [review]
8988994: Fix crash by preventing a corrupt tree when closing a subtree in IMAP/NEWS subscribe dialog

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

Thanks, I could reproduce the crash on the nntp.aioe.org server and this patch fixes it.

::: mailnews/base/src/nsSubscribableServer.cpp
@@ +985,5 @@
>  {
>    SubscribeTreeNode *node = mRowMap[aIndex];
>    if (node->isOpen)
>    {
>      // Close the node by finding the next sibling

This comment should probably go.
Attachment #8988995 - Flags: review?(acelists) → review+
Removed comment as per comment #8 and other tweaks, like s/if(/if (/ and fixed comment. Use interdiff for details.
Attachment #8988995 - Attachment is obsolete: true
Attachment #8989014 - Flags: review+
Attachment #8989014 - Flags: approval-comm-esr60+
Attachment #8989014 - Flags: approval-comm-beta+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7242e9eae2c3
Fix crash by preventing a corrupt tree when closing a subtree in IMAP/News subscribe dialog. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
You need to log in before you can comment on or make changes to this bug.