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

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: jerry)

Tracking

(Blocks 1 bug, {crash, regression})

58 Branch
mozilla59
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox57 wontfix, firefox58 fixed, firefox59 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

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&lt;mozilla::DisplayItemClip::RoundedRect, nsTArrayInfallibleAllocator&gt;::operator==&lt;nsTArrayInfallibleAllocator&gt; 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&lt;&lt;lambda_593feee20c50667d87c0fe33c81e3e5b&gt; &gt; 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.
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
Assignee: nobody → hshih
Blocks: 1388995
Status: NEW → ASSIGNED
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.
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
Attachment #8928933 - Flags: review?(matt.woodrow)
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.
Attachment #8928933 - Flags: review?(matt.woodrow) → review+
Attachment #8928933 - Attachment is obsolete: true
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0fbb755131
make sure we could always get the specific userData from mask layer. r=mattwoodrow
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5d0fbb755131
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Hi Jerry,
Do you think this is worth uplifting to 58 if it's not too risky?
Flags: needinfo?(hshih)
Yes, I don't see the crash from 59 after 11/18(comment 9).
Flags: needinfo?(hshih)
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
Attachment #8929288 - Flags: approval-mozilla-beta?
Comment on attachment 8929288 [details] [diff] [review]
make sure we could always get the specific userData from mask layer. v2

Fix a crash. Beta58+.
Attachment #8929288 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.