1461267 - Crash in InvalidArrayIndex_CRASH | MergeState::Finalize

Status: RESOLVED WORKSFORME

(Core :: Web Painting, defect, P3)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-efc900b1-21a1-4077-a46c-b4c0f0180511.
=============================================================

Top 10 frames of crashing thread:

0 mozglue.dll MOZ_CrashPrintf mfbt/Assertions.cpp:63
1 xul.dll InvalidArrayIndex_CRASH xpcom/ds/nsTArray.cpp:26
2 xul.dll MergeState::Finalize layout/painting/RetainedDisplayListBuilder.cpp:303
3 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:491
4 xul.dll MergeState::ProcessItemFromNewList layout/painting/RetainedDisplayListBuilder.cpp:280
5 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:488
6 xul.dll RetainedDisplayListBuilder::AttemptPartialUpdate layout/painting/RetainedDisplayListBuilder.cpp:1172
7 xul.dll nsLayoutUtils::PaintFrame layout/base/nsLayoutUtils.cpp:3679
8 xul.dll mozilla::PresShell::Paint layout/base/PresShell.cpp:6351
9 xul.dll nsViewManager::ProcessPendingUpdatesPaint view/nsViewManager.cpp:480

=============================================================

There are 23 crashes (from 20 installations) in nightly 62 starting with buildid 20180510220127.
:mattwoodrow, could you investigate please ?

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Bug 1453541 is targeting Fx61, so calling this affected to get it on the radar.

Comment 2

[Matt Woodrow (:mattwoodrow)]

I believe this is a duplicate of bug 1459997 (just a slightly different callstack, but same cause), but we can leave it separate for now.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Still seeing a few crashes here in beta 61 and nightly 62.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> I believe this is a duplicate of bug 1459997 (just a slightly different
> callstack, but same cause), but we can leave it separate for now.

Like bug 1461832, this also affects 61.0b13 beta channel builds, so we'll presumably be seeing this on release as well.

Comment 5

[Matt Woodrow (:mattwoodrow)]

This one is interesting. We iterate mOldDAG, and use the index to lookup entries in mOldList. The assertion is an invalid array invalid, so our two structures must be different lengths.

The crashing index is often, but not always 0, so it really seems like they got out of sync somehow rather than just being uninitialized (or there's two bugs).

I've done an audit of the code and can't see any way this could happen :(

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1461267 - Add some more assertions to verify the length of the DAG.

Review commit: https://reviewboard.mozilla.org/r/251546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251546/

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8986088 [details]
Bug 1461267 - Add some more assertions to verify the length of the DAG.

https://reviewboard.mozilla.org/r/251546/#review258166

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e0e314fe39a
Add some more assertions to verify the length of the DAG. r=mstange

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9e0e314fe39a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

As expected, this went away on 61.0b14 when we disabled RDL by default. Interestingly, there haven't been any reports from 62 yet (and b1/b2 were DevEdition-only). Matt, did this signature morph into something else now?

Comment 11

[Matt Woodrow (:mattwoodrow)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> As expected, this went away on 61.0b14 when we disabled RDL by default.
> Interestingly, there haven't been any reports from 62 yet (and b1/b2 were
> DevEdition-only). Matt, did this signature morph into something else now?

Yes, the new assertions will have given it a new signature.

I found two crashes:

https://crash-stats.mozilla.com/report/index/f3d3b0be-7dd2-45c6-9d69-194f00180625
https://crash-stats.mozilla.com/report/index/58c4fbd2-0a3f-4a4e-bbb0-fcdbe0180627

Both have the DAG with length of 2, and neither have the list length available in the minidump. Both are lists of type nsDisplayWrapList.

Crashing in PreProcessDisplayList suggests that someone modified our list between paints. Crashing in MergeState suggests that someone modified our list while we were building the new list?! Or possibly that we managed to not call ProProcessDisplayList for this list?

Both very weird, and I've audited for this pattern before and not found anything.

No URLs available as of yet.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

No crashes in 62 yet with those signatures, so I'm dropping the tracking flag.

Comment 13

[Gabriele Svelto [:gsvelto]]

I found a new signature for this, the stacks look almost identical to some of the other signatures in this bug.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Pretty low volume in 62 release, moderate on beta 63 and we are about to release 63. There are only a few crashes in nightly 64.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

This is P1 but has been wontfixed several times in a row. Marking this fix-optional to remove it from repeated triage.

Comment 16

[Marcia Knous [:marcia]]

Updating flags. Still happening across 67/68 but in low volume.

Comment 17

[Pascal Chevrel:pascalc]

we are less than a week away from RC so wontfix for 67.

Comment 18

[Pascal Chevrel:pascalc]

Matt, this is marked as a P1 but we have shipped multiple releases with this bug and the volume of crashes is low for these signatures, should be downgrade it to P3?

Comment 19

[Matt Woodrow (:mattwoodrow)]

Yeah, definitely. I'm not actively working on this due to the low volume (though I would if a testcase/STR surfaced).

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Happy to take a patch for 70 but since this is triaged and set to P3 priority I'm setting it as fix-optional.
That will remove the bug from weekly regression triage.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:mattwoodrow, maybe it's time to close this bug?

Comment 22

[Gabriele Svelto [:gsvelto]]

I had a quick look at the crashes and the only signature that still sees active crashes is [@ RetainedDisplayListBuilder::PreProcessDisplayList]. Under that signatures there's at least three different stacks with significantly different causes. The most common of which is hitting this assertion. The second most common is this assertion but it's a lot less frequent. Since the remaining crashes don't seem to have anything to do with the original issue I suggest to close this bug and open a new one specifically under the [@ RetainedDisplayListBuilder::PreProcessDisplayList] signature, pointing out the failing assertions.

Comment 23

[Andrei Purice]

Closing this issue as Resolved > Worksforme since no crashes with this Signature were reported in the last 6 months.

Comment 24

[Calixte Denizet (:calixte)]

:andrei, what's the signature you're talking about ?