1416868 - Crash in nsTArray_Impl<T>::operator==<T> | mozilla::MaskLayerUserData::operator==

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-7883f933-ee6b-4169-b60a-534b30171113.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsTArray_Impl<mozilla::DisplayItemClip::RoundedRect, nsTArrayInfallibleAllocator>::operator==<nsTArrayInfallibleAllocator> xpcom/ds/nsTArray.h:969
1 xul.dll mozilla::MaskLayerUserData::operator== layout/painting/FrameLayerBuilder.cpp:1593
2 xul.dll mozilla::ContainerState::CreateMaskLayer layout/painting/FrameLayerBuilder.cpp:6363
3 xul.dll mozilla::ContainerState::SetupMaskLayer layout/painting/FrameLayerBuilder.cpp:6324
4 xul.dll mozilla::ContainerState::ProcessDisplayItems layout/painting/FrameLayerBuilder.cpp:4317
5 xul.dll mozilla::FrameLayerBuilder::BuildContainerLayerFor layout/painting/FrameLayerBuilder.cpp:5678
6 xul.dll nsDisplayOpacity::BuildLayer layout/painting/nsDisplayList.cpp:6261
7 xul.dll mozilla::FrameLayerBuilder::AddPaintedDisplayItem layout/painting/FrameLayerBuilder.cpp:4700
8 xul.dll mozilla::ContainerState::FinishPaintedLayerData<<lambda_593feee20c50667d87c0fe33c81e3e5b> > layout/painting/FrameLayerBuilder.cpp:3164
9 xul.dll mozilla::PaintedLayerDataNode::PopPaintedLayerData layout/painting/FrameLayerBuilder.cpp:2869

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

crash reports with this signature are rising in volume during the 58 cycle - most of the time they are happening in the content process.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Maybe we could have a nullptr checking before de-reference the userData.

https://hg.mozilla.org/releases/mozilla-beta/annotate/1c336e874ae8/layout/painting/FrameLayerBuilder.cpp#l6363

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

nullptr check sounds great - should we add a gfxCriticalError/Note or some such if this is really unexpected?  That way we won't crash, but we can see if something went wrong.

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: make sure we could always the specific userData from mask layer.

The gecko have two types of mask layer: css mask layer and the regular mask layer.
The hash key of ContainerState::mRecycledMaskImageLayers doesn't contain mask type.
So, we might get a css mask layer when we try to get a regular mask layer. Then, we
will get a nullptr of userData.
This patch add a userData checking in ContainerState::CreateOrRecycleMaskImageLayerFor()
to avoid the problem.

MozReview-Commit-ID: EEUhkctqwR2

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8928933 [details] [diff] [review]
make sure we could always the specific userData from mask layer.

Review of attachment 8928933 [details] [diff] [review]:
-----------------------------------------------------------------

The regular mask layer and css mask layer will be collected here:
https://hg.mozilla.org/mozilla-central/annotate/fc194660762d1b92e1679d860a8bf41116d0f54f/layout/painting/FrameLayerBuilder.cpp#l4922

We might have the collision for these mask layers. Then, we can't get the correct user data.

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: make sure we could always get the specific userData from mask layer. v2

update the patch comment.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4530ad9cd83e9387856574b4b9f49cbcf9c6a047

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land the attachment 8929288 [details] [diff] [review] to m-c.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0fbb755131
make sure we could always get the specific userData from mask layer. r=mattwoodrow

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5d0fbb755131

Comment 10

[Gerry Chang [:gchang]]

Hi Jerry,
Do you think this is worth uplifting to 58 if it's not too risky?

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Yes, I don't see the crash from 59 after 11/18(comment 9).

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8929288 [details] [diff] [review]
make sure we could always get the specific userData from mask layer. v2

Approval Request Comment

[Feature/Bug causing the regression]:
Maybe it a regression from bug 1234485. It create a new type of mask. But that's landed in mozilla52. I don't know why the crash rate becomes higher in mozilla58.
 
[User impact if declined]:
Hit Crash at content side when gecko try to recycle a mask layer.

[Is this code covered by automated tests?]:
Yes. We have reftest for mask.

[Has the fix been verified in Nightly?]:
I think yes. I don't see the new crash after this patch landed.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It add a nullptr checking for the result from a recycler. I think it doesn't change too much.

[String changes made/needed]:
none

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8929288 [details] [diff] [review]
make sure we could always get the specific userData from mask layer. v2

Fix a crash. Beta58+.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6036ae55ded0