Crash in InvalidArrayIndex_CRASH | nsSHistory::RemoveFromExpirationTracker

RESOLVED FIXED in Firefox 55

Status

()

Core
Document Navigation
--
critical
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: calixte, Assigned: freesamael)

Tracking

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

55 Branch
mozilla55
Unspecified
Windows 7
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [clouseau], crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

9 months ago
This bug was filed from the Socorro interface and is 
report bp-45057a9f-1caf-4597-9925-1d5030170507.
=============================================================

There are 9 crashes (from 3 installations) in nightly 55, it's beginning with buildid 20170506030204. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1347823.

[1] https://hg.mozilla.org/mozilla-central/rev?node=f8bebf994dbfef3d091d61b9906854736cc12ffc
Flags: needinfo?(sawang)
In all of these crashes, the index is -1 and the length is 0.
I've definitely made some mistakes.
Assignee: nobody → sawang
Flags: needinfo?(sawang)
I still have no clue why this is happening. I saw some bug reports with uptime less than a minutes which mostly implies HistoryTracker::NotifyExpired hans't even been invoked as the timeout value is 30mins. 

I'm making try runs with some MOZ_ASSERTs. Hopefully to get more clues.
Unfortunately these MOZ_ASSERT didn't show me a possible crash cause (but I find another bug related to my patch that nsSHEntryShared::mSHistory may not be init in some cases).

Do the addons show on bug reports are all enabled addons? I noticed an interesting correlation that all installations of those crashes have DOM Inspector and UserChromeJS which are marked as not multiprocess compatible and should be disabled by default on nightly. I don't know how or if that contributes to the crashes, tho.
Created attachment 8867977 [details] [diff] [review]
Add MOZ_DIAGNOSTIC_ASSERTs for bug hunting

MozReview-Commit-ID: JPnkkJlkwDY
Comment on attachment 8867977 [details] [diff] [review]
Add MOZ_DIAGNOSTIC_ASSERTs for bug hunting

I didn't manage to reproduce the issue. The only possible problems I can think of is either the tracking state was somehow incorrect, or the HistoryTracker associated to the entry had somehow been changed.

Do you think it's OK to add some MOZ_DIAGNOSTIC_ASSERTs for hunting or is there other suggestions that I can try?
Attachment #8867977 - Flags: review?(bugs)

Comment 7

8 months ago
Comment on attachment 8867977 [details] [diff] [review]
Add MOZ_DIAGNOSTIC_ASSERTs for bug hunting

Looks reasonable.
Attachment #8867977 - Flags: review?(bugs) → review+
This is not a fix, but it could help getting more clues on nightly.
Keywords: checkin-needed, leave-open

Comment 9

8 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5e3003f692
Add MOZ_DIAGNOSTIC_ASSERTs for bug hunting. r=smaug
Keywords: checkin-needed
(Reporter)

Comment 11

8 months ago
There are 7 crashes from the same installation (and 3 in Seamonkey) with signature "nsSHEntry::SetSHistory".
Crash Signature: [@ InvalidArrayIndex_CRASH | nsSHistory::RemoveFromExpirationTracker] → [@ InvalidArrayIndex_CRASH | nsSHistory::RemoveFromExpirationTracker] [@ nsSHEntry::SetSHistory ]
(Reporter)

Updated

8 months ago
Duplicate of this bug: 1365897
So there does exist cases that nsSHEntryShared::mSHistory is overwritten for some reasons (!!!). 

The only place we're setting nsSHEntryShared::mSHistory is nsSHistory::AddEntry(); we don't copy that data member in nsSHEntryShared::Duplicate() or nsSHEntry::Clone(), neither provide scriptable XPCOM interface to set it. The only explanation I can think of is that one SHEntry has been moved from a SHistory to another...
Created attachment 8869325 [details] [diff] [review]
v0

1. Convert MOZ_DIAGNOSTIC_ASSERTs previously added for bug hunting to normal
   MOZ_ASSERTs.
2. Clone the entry and abandon its bfcache if the entry was associated to a
   different SHistory when calling nsSHistory::AddEntry/ReplaceEntry.
3. Support associating to a different nsSHistory instance when duplicate
   nsSHEntryShared for the case mentioned above, and for nsDocShell::LoadPage
   (in which the entry passed in is also from another SHistory).

MozReview-Commit-ID: 54y3jpWGDsd
(Assignee)

Updated

8 months ago
Attachment #8867977 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8869325 - Attachment description: Fix some edge cases where nsSHEntryShared::mSHistory could be incorrct → v0
Attachment #8869325 - Attachment is obsolete: true
Created attachment 8869326 [details] [diff] [review]
Fix some edge cases where nsSHEntryShared::mSHistory could be incorrct
Comment on attachment 8869326 [details] [diff] [review]
Fix some edge cases where nsSHEntryShared::mSHistory could be incorrct

My proposal is to clone entry & abandon bfcache for this tricky case. 

I also fixed nsDocShell::LoadPage, as well as ReplaceEntry case. Both were caught on try after I added a MOZ_ASSERT to check if top level bfcache entries always have mSHistory set. 

Would you take a look?
Attachment #8869326 - Flags: review?(bugs)

Comment 18

8 months ago
Comment on attachment 8869326 [details] [diff] [review]
Fix some edge cases where nsSHEntryShared::mSHistory could be incorrct

This looks very very complicated. Why we need to do all this work?

I don't like cloning when calling nsSHistory::AddEntry. One may still keep using the original shentry, but session history doesn't use it. The end result is quite surprising.
Attachment #8869326 - Flags: review?(bugs) → review-
FWIW: frg built an experimental SeaMonkey for L64 with attachment 8869326 [details] [diff] [review] patched in as well as his r+ attachment 8867467 [details] [diff] [review] for bug 1364677. It does not exhibit the crash which I get at every startup in the same profile with the current trunk nightlies.

If anyone wants to try it (L64 only AFAIK, and of course at your own risk), it's http://www.pinballz.net/custom/frg/seamonkey-2.52a1.en-US.linux-x86_64.tar.bz2
>> If anyone wants to try it (L64 only AFAIK, and of course at your own risk), it's 

Sorry already deleted it. Can't use this space/domain for more than short term file transfers. Only pinball related is allowed to stay longer :)

FRG
(In reply to Frank-Rainer Grahl from comment #20)
> >> If anyone wants to try it (L64 only AFAIK, and of course at your own risk), it's 
> 
> Sorry already deleted it. Can't use this space/domain for more than short
> term file transfers. Only pinball related is allowed to stay longer :)
> 
> FRG

Ah. I still have a copy, but not enough room on my userspace at my ISP's. I suppose anyone interested could send me (or you) an email and get it back as an attachment (but 54886827 bytes).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 8869326 [details] [diff] [review]
> Fix some edge cases where nsSHEntryShared::mSHistory could be incorrct
> 
> This looks very very complicated. Why we need to do all this work?
> 
> I don't like cloning when calling nsSHistory::AddEntry. One may still keep
> using the original shentry, but session history doesn't use it. The end
> result is quite surprising.

Let me try a different approach - rejecting AddEntry / ReplaceEntry if we know it's associated to another shistory. For Seamonkey's case I filed bug 1366643 to fix on the caller side; while for add-on's case it will encounter a failure. These XPCOM addons won't work in 57 anyway.
(Assignee)

Updated

8 months ago
Attachment #8869326 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8869936 - Attachment is obsolete: true
Attachment #8869936 - Flags: review?(bugs)
Comment hidden (mozreview-request)
Filed a separated bug 1366691 for the case where mSHistory is not set.

Comment 28

8 months ago
So why/how did bug 1347823 cause this?
(In reply to Olli Pettay [:smaug] from comment #28)
> So why/how did bug 1347823 cause this?

My theory is:

1. nsSHEntryShared::SetContentViewr was called when it's on SHistory A. It's then added to HistoryTracker A and marked as IsTracked().

2. The entry was moved to another SHistory B, nsSHEntryShared::mSHistory got overwritten.

3. nsSHEntryShared::DropPresentationState() got called, it asked SHistory B to remove itself from HistoryTracker B, which was actually empty.

4. It got crash at [1] - generation.Length() was 0, last became 2^32-1, generation[last] triggered InvalidArrayIndex_CRASH.

That wouldn't happen before bug 1347823 since nsSHEntryShared was using a per-process global gHistoryTracker.

[1] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/xpcom/ds/nsExpirationTracker.h#185-186
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 34

8 months ago
mozreview-review
Comment on attachment 8869934 [details]
Bug 1363036 - Part 1: Remove MOZ_DIAGNOSTIC_ASSERTs previously added for bug hunting.

https://reviewboard.mozilla.org/r/141498/#review145568
Attachment #8869934 - Flags: review?(bugs) → review+

Comment 35

8 months ago
mozreview-review
Comment on attachment 8869935 [details]
Bug 1363036 - Part 2: Reject AddEntry / ReplaceEntry if the entry has been associated to another SHistory. Cleanup mHistoryTracker if root docshell changes.

https://reviewboard.mozilla.org/r/141500/#review145570

I guess I could live with that. Hopefully SM folks aren't too annoyed.

::: docshell/shistory/nsISHEntry.idl:338
(Diff revision 4)
>       * current process. This flag does not survive a browser process switch.
>       */
>      readonly attribute boolean loadedInThisProcess;
>  
>      /**
>       * Set the session history it belongs to. It's only set on root entries.

Want to update the comment to indicate this isn't just about setting the history.
Attachment #8869935 - Flags: review?(bugs) → review+
Blocks: 1345770
Comment hidden (mozreview-request)

Comment 38

8 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/68e0dc399cf7
Part 1: Remove MOZ_DIAGNOSTIC_ASSERTs previously added for bug hunting. r=smaug
https://hg.mozilla.org/integration/autoland/rev/98b85a78d45f
Part 2: Reject AddEntry / ReplaceEntry if the entry has been associated to another SHistory. Cleanup mHistoryTracker if root docshell changes. r=smaug
Keywords: checkin-needed

Comment 39

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/68e0dc399cf7
https://hg.mozilla.org/mozilla-central/rev/98b85a78d45f
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
Blocks: 1396527
You need to log in before you can comment on or make changes to this bug.