1451384 - Crash in InvalidArrayIndex_CRASH | MergeState::ProcessItemFromNewList

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-fcedef10-73f2-489c-bcf3-f18630180404.
=============================================================

Seen while looking at nightly crash data: https://bit.ly/2q4PL1j. Windows crashes which started in Build 20180404100127. Crash reason ElementAt(aIndex = 0, aLength = 0).

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:277
3 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:463
4 xul.dll MergeState::ProcessItemFromNewList layout/painting/RetainedDisplayListBuilder.cpp:264
5 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:463
6 xul.dll MergeState::ProcessItemFromNewList layout/painting/RetainedDisplayListBuilder.cpp:264
7 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:463
8 xul.dll MergeState::ProcessItemFromNewList layout/painting/RetainedDisplayListBuilder.cpp:264
9 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:463

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

Comment 1

[Jim Mathies [:jimm]]

STR:

Win7, 20180404100127 Nightly

1) Open google spread sheet with a link is a cell
2) click on tyhe cell, then click on the popup link

result: spreadsheet tab crashes
https://crash-stats.mozilla.com/report/index/eef70ded-7854-43c2-8b22-f19890180404#tab-details

Comment 2

[Chris Peterson [:cpeterson]]

I hit this crash with the same STR as Jim. I can't reproduce the crash in a clean profile, though. I spend a LOT of my day in Google spreadsheets and only saw this crash today, so this is probably a very recent regression.

Comment 3

[Marcia Knous [:marcia]]

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=00bdc9451be6557ccce1492b9b966d4435615380&tochange=ff0efa4132f0efd78af0910762aec7dcc1a8de66

Comment 4

[Jim Jeffery not reading bug-mail 1/2/11]

This site is also crashing with same sig just loading page and waiting a few seconds.

Today's Nightly  cset: https://hg.mozilla.org/mozilla-central/rev/ff0efa4132f0efd78af0910762aec7dcc1a8de66
Win10 x64

Comment 5

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #4)
> This site is also crashing with same sig just loading page and waiting a few
> seconds.
> 
> Today's Nightly  cset:
> https://hg.mozilla.org/mozilla-central/rev/
> ff0efa4132f0efd78af0910762aec7dcc1a8de66
> Win10 x64

Nevermind, I think it was a cookie issue.  site has been redesigned and I had some stale cookies perhaps.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1451384 - Check IsChanged on the old item during merging, since that's the one that might have a deleted frame.

This happens when an nsIFrame* that builds an nsDisplayWrapList is deleted, but then the memory is immediately reused for another frame that builds the same type display item, within the same display list.

PreProcessDisplayLists chooses not to descend into the nsDisplayWrapList for the deleted frame, and so mOldItems remains uninitialized for the old sublist.
When adding the new instance, IsChanged returns false, since the pointers are the same, and we're checking HasDeletedFrame on the new instance (where it's never true), instead of the old. We then recurse into MergeDisplayLists, with an uninitialized mOldItems array, and crash.

I haven't added a test because I haven't yet figured out how to create a minimal testcase, and the test would rely on implementation details of the frame allocator to remain unchanged to be useful.

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

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8965165 [details]
Bug 1451384 - Check IsChanged on the old item during merging, since that's the one that might have a deleted frame.

https://reviewboard.mozilla.org/r/233846/#review239516

Oh, I see, so IsChanged must never be called on a new item, only on an old item. Should we make the changed state part of the OldItemInfo, then? Having an IsChanged method that accepts any nsDisplayItem* might be prone to misuse.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9c5a53970bf
Check IsChanged on the old item during merging, since that's the one that might have a deleted frame. r=mstange

Comment 10

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #8)
> Comment on attachment 8965165 [details]
> Bug 1451384 - Check IsChanged on the old item during merging, since that's
> the one that might have a deleted frame.
> 
> https://reviewboard.mozilla.org/r/233846/#review239516
> 
> Oh, I see, so IsChanged must never be called on a new item, only on an old
> item. Should we make the changed state part of the OldItemInfo, then? Having
> an IsChanged method that accepts any nsDisplayItem* might be prone to misuse.

I've landed as-is, since I had a clean try run and I want to fix the google sheets regression.

That's a great idea, will do a follow-up to fix it.

Comment 11

[Davide Aversa]

I am having this issue a lot in the last couple of builds (Windows 10, Nightly). This happens to me when I click on a lot of text fields. For now it happens consistently on GMail, Twitter, Reddit and YouTube. If there is something else I can provide, I 'd be happy to help.

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a9c5a53970bf

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8965165 [details]
Bug 1451384 - Check IsChanged on the old item during merging, since that's the one that might have a deleted frame.

https://reviewboard.mozilla.org/r/233846/#review240046

::: layout/tools/reftest/runreftest.py:242
(Diff revision 1)
>          self.resolver = self.resolver_cls()
>          self.log = None
>          self.outputHandler = None
>          self.testDumpFile = os.path.join(tempfile.gettempdir(), 'reftests.json')
>  
> -        self.run_by_manifest = True
> +        self.run_by_manifest = False

This seems like a local testing change that you didn't intend to land?

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 8965165 [details]
Bug 1451384 - Check IsChanged on the old item during merging, since that's the one that might have a deleted frame.

https://reviewboard.mozilla.org/r/233846/#review240046

> This seems like a local testing change that you didn't intend to land?

Yes, this needs to be reverted. It'll cause a large spike of intermittents (on Windows 7 especially).

Comment 15

[Andrew Halberstadt [:ahal]]

Backed out just that hunk on central:
https://hg.mozilla.org/mozilla-central/rev/0d661c592a164ca918ed12f87cbcf7f52c293359

Comment 16

[Markus Stange [:mstange]]

Thanks Andrew!

I noticed that hunk during the review and thought I had commented on it, but apparently I didn't...

Comment 17

[Matt Woodrow (:mattwoodrow)]

Oops! Thanks for fixing it!