Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::RemoveElementsAt | nsSHistory::AddEntry

RESOLVED FIXED in Firefox 63



11 months ago
11 months ago


(Reporter: calixte, Assigned: njn)


(Blocks 1 bug, {crash, regression})

Windows 10
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63 fixed)


(crash signature)


(1 attachment)

This bug was filed from the Socorro interface and is
report bp-ce78c9b8-bcf2-4c50-852d-27c440180830.

Top 10 frames of crashing thread:

0 mozglue.dll MOZ_CrashPrintf mfbt/Assertions.cpp:67
1 xul.dll InvalidArrayIndex_CRASH xpcom/ds/nsTArray.cpp:26
2 xul.dll void nsTArray_Impl<nsCOMPtr<nsISHTransaction>, nsTArrayInfallibleAllocator>::RemoveElementsAt xpcom/ds/nsTArray.h:2389
3 xul.dll nsSHistory::AddEntry docshell/shistory/nsSHistory.cpp:663
4 xul.dll nsresult nsDocShell::AddToSessionHistory docshell/base/nsDocShell.cpp:12223
5 xul.dll bool nsDocShell::OnNewURI docshell/base/nsDocShell.cpp:11524
6 xul.dll nsDSURIContentListener::DoContent docshell/base/nsDSURIContentListener.cpp:196
7 xul.dll bool nsDocumentOpenInfo::TryContentListener uriloader/base/nsURILoader.cpp:759
8 xul.dll nsDocumentOpenInfo::OnStartRequest uriloader/base/nsURILoader.cpp:306
9 xul.dll void mozilla::net::HttpChannelChild::DoOnStartRequest netwerk/protocol/http/HttpChannelChild.cpp:765


There are 8 crashes (from 7 installations) in nightly 63 with buildid 20180830111745. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1486358.

Flags: needinfo?(n.nethercote)
In all of these crashes we get into AddEntry() with mTransactions.Length() == 0 but mIndex is positive, e.g 3, 7, 22.

Then we call mTransactions.TruncateLength(mIndex + 1) and the bounds check fails because TruncateLength() requires its argument to be less than or equal to the length.

So, things have gone awry prior to the call to AddEntry(). There are two possibilities.

- Bug 1486358 introduced a defect.

- Bug 1486358 exposed a latent defect. The old code would not have aborted with mLength==0 and positive mIndex. (AddEntry() would have exited having given mLength a bogus value of mIndex+1, though.)

I'm leaning towards the latter, but that doesn't really help. I can't see how to fix this so I'll probably have to back bug  1486358 out. After that I might add some diagnostic asserts at various places to check that mLength and mIndex are reasonable, to determine if this really is a latent defect.
Flags: needinfo?(n.nethercote)
I did some digging, by adding a bunch of assertions that checked the transactions indices and length, and I found a latent defect: mRequestedIndex can get out of bounds, because in PurgeHistory() we update mIndex if necessary, but we don't do likewise for mRequestedIndex. And mRequestedIndex gets assigned to mIndex in UpdateIndex().

So this may explain how mIndex is getting out of bounds. I am not certain that this latent defect is causing the crashes, but it's my best theory at the moment. I will land a patch fixing that and see what happens to the crash rates in Nightly. If they stop, good; if not, I will back out bug 1486358's patches.
Assignee: nobody → n.nethercote
Pushed by
Keep mRequestedIndex in bounds in PurgeHistory(). r=me
I have landed this ahead of getting r+; apologies for the forwardness, but I want to get it into the earliest possible Nightly.
Priority: -- → P1
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Let's leave this open until we have confirmation in a day or two whether the crashes have stopped.
Resolution: FIXED → ---
Attachment #9005555 - Flags: review?(nika) → review+
Nightly 20180830111745 and 20180830220110 had these crashes. Nightly 20180831100058 had these patches, and there have been no crashes in that build or later Nightly builds. Time to declare victory!
Closed: 11 months ago11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.