1459997 - Crash in InvalidArrayIndex_CRASH | MergeState::ProcessItemFromNewList

Status: RESOLVED DUPLICATE of bug 1462497

(Core :: Web Painting, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

+++ This bug was initially created as a clone of Bug #1456534 +++

It would appear that bug 1456534 made a significant impact on the crash rate, but some crashes still remain.

From report bp-3d604bf2-e52a-4088-bbed-858790180508, the 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::ProcessItemFromNewList layout/painting/RetainedDisplayListBuilder.cpp:274
3 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:487
4 xul.dll RetainedDisplayListBuilder::AttemptPartialUpdate layout/painting/RetainedDisplayListBuilder.cpp:1097
5 xul.dll nsLayoutUtils::PaintFrame layout/base/nsLayoutUtils.cpp:3679
6 xul.dll mozilla::PresShell::Paint layout/base/PresShell.cpp:6351
7 xul.dll nsViewManager::ProcessPendingUpdatesPaint view/nsViewManager.cpp:480
8 xul.dll nsViewManager::ProcessPendingUpdatesForView view/nsViewManager.cpp:412
9 xul.dll nsViewManager::ProcessPendingUpdates view/nsViewManager.cpp:1102

And a Linux report: bp-c985105b-d99b-4af4-b938-150cd0180507

Comment 1

[Ryan VanderMeulen [:RyanVM]]

This is the #2 top content crash on Beta61 at the moment.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1459997 - Add much more rigorous assertions for retained-dl assumptions.

This adds an assertion checking for duplicate items whenever we create an item, and when we merge an item into the final list.

I had to disable tracking for the anonymous inner list for nsDisplayPerspective and nsDisplayTransform (and manually forward RemoveFrame to them), as well as skipping the assertion for multi-page (since we can end up duplicating wrap lists, but isn't a problem, since we don't retain these).

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

Comment 3

[Matt Woodrow (:mattwoodrow)]

Added a patch with better assertions to try track down why this happens.

Bug 1461231 and bug 146052 fix a couple of cases where this happened on treeherder, so we might be in better state once those land.

Comment 4

[Miko Mynttinen]

Comment on attachment 8975708 [details]
Bug 1459997 - Add much more rigorous assertions for retained-dl assumptions.

https://reviewboard.mozilla.org/r/243944/#review250012

LGTM.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff93cd94b7c5
Add much more rigorous assertions for retained-dl assumptions. r=miko

Comment 6

[Andreea Pavel [:apavel]]

Backed out for reftest assertion failures at builds/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:141

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ff93cd94b7c548ef57fa13b7eaf85992a0a60587
 
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=178675358&repo=autoland&lineNumber=6164

Backout: https://hg.mozilla.org/integration/autoland/rev/89a073a0ff2c179b604a11879c4c1408a42a4b63

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This is green on top of the other RDL patches now safely landed on autoland.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9d7c241996b2a863dce5d5b2f510582c4f81289

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/488b7be0348b
Add much more rigorous assertions for retained-dl assumptions. r=miko

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8975708 [details]
Bug 1459997 - Add much more rigorous assertions for retained-dl assumptions.

Approval Request Comment
[Feature/Bug causing the regression]: Retained-dl
[User impact if declined]: These just add some new assertions (MOZ_DIAGNOSTIC_ASSERT, so will only affect dev edition), that will hopefully make it easier to pinpoint any remaining issues.
[Is this code covered by automated tests?]: All issues uncovered by the new assertions are fixed.
[Has the fix been verified in Nightly?]: Nothing to verify.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: All the other retained-dl uplifts, to ensure that none of the assertions get hit on automation.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just adds new assertions, not compiled into actual beta builds.
[String changes made/needed]: None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975708 [details]
Bug 1459997 - Add much more rigorous assertions for retained-dl assumptions.

Stronger assertions which will hopefully help us narrow down some of the retained display list crashes currently being reported. Useful even if they'll only be active in DevEdition. Approved for 61.0b6.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975708 [details]
Bug 1459997 - Add much more rigorous assertions for retained-dl assumptions.

https://hg.mozilla.org/releases/mozilla-beta/rev/233d36dfd5c9

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/488b7be0348b

Comment 13

[Calixte Denizet (:calixte)]

There are 2 crashes with signature "nsDisplayItem::GetOldListIndex".
The moz_crash_reason is "Item found was in the wrong list! type 72".

Comment 14

[[:philipp]]

adding the 64bit signature with the same moz_crash_reason too...

Comment 15

[Matt Woodrow (:mattwoodrow)]

Bug 1462497 also exists for this new signature.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975708 [details]
Bug 1459997 - Add much more rigorous assertions for retained-dl assumptions.

Clearing the Beta approval here just to get it off the needs-uplift radar.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Still the #3 top overall content crash signature on Beta61. From what I'm seeing, it's mostly variations of the ElementAt assertion.

https://crash-stats.mozilla.com/report/index/b34595c6-e697-42f6-9cba-3a5780180527
https://crash-stats.mozilla.com/report/index/6aae47e2-c3d2-41f3-8082-cb8c90180527
https://crash-stats.mozilla.com/report/index/461e8989-5cc1-4795-98fe-e147b0180527
https://crash-stats.mozilla.com/report/index/b2bd6d57-d9f2-49f3-907c-e33ed0180527
https://crash-stats.mozilla.com/report/index/67e302ae-4d13-4fd0-8d2b-869330180527

https://crash-stats.mozilla.com/report/index/94fd8c78-b813-49d7-86d5-3cd580180526
https://crash-stats.mozilla.com/report/index/f76f4271-7159-4b69-b07f-f29c00180524
https://crash-stats.mozilla.com/report/index/67c6ce76-7f80-4812-abf7-340a80180521

Matt, do any of these recent reports tell you anything useful? It is interesting to me that we haven't seen any Nightly reports since 16-May, though Beta still has reports from as recently as last Friday's b8.

Comment 18

[Matt Woodrow (:mattwoodrow)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Still the #3 top overall content crash signature on Beta61. From what I'm
> seeing, it's mostly variations of the ElementAt assertion.
> 
> https://crash-stats.mozilla.com/report/index/b34595c6-e697-42f6-9cba-
> 3a5780180527
> https://crash-stats.mozilla.com/report/index/6aae47e2-c3d2-41f3-8082-
> cb8c90180527
> https://crash-stats.mozilla.com/report/index/461e8989-5cc1-4795-98fe-
> e147b0180527
> https://crash-stats.mozilla.com/report/index/b2bd6d57-d9f2-49f3-907c-
> e33ed0180527
> https://crash-stats.mozilla.com/report/index/67e302ae-4d13-4fd0-8d2b-
> 869330180527
> 
> https://crash-stats.mozilla.com/report/index/94fd8c78-b813-49d7-86d5-
> 3cd580180526
> https://crash-stats.mozilla.com/report/index/f76f4271-7159-4b69-b07f-
> f29c00180524
> https://crash-stats.mozilla.com/report/index/67c6ce76-7f80-4812-abf7-
> 340a80180521
> 
> Matt, do any of these recent reports tell you anything useful? It is
> interesting to me that we haven't seen any Nightly reports since 16-May,
> though Beta still has reports from as recently as last Friday's b8.

The assertions added here are MOZ_DIAGNOSTIC_ASSERT, so they don't affect beta. On Nightly, all the crashes have moved to the HasMatchingItemInOldList signature, on beta this patch had no effect.

Bug 1462497 has actual fixes for the most common causes, along with a patch to avoid crashing for the remainder. I expect to see the beta crash rate drop significantly once that lands!

Comment 19

[Matt Woodrow (:mattwoodrow)]

There are no crashes in 61.0b9, so it looks like bug 1462497 fixed the actual issue here.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Where are we tracking the crashes on trunk with this signature still?

Comment 21

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

Matt, sorry to bug you here but it looks like there are ~400 crashes a week still on beta 62. That's fairly high. Should we file a new bug for those crashes/specific signatures? Or just keep this one open?

Comment 22

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

Actually - looks like this is already covered, I missed that this is a duplicate bug.