Open Bug 1251615 Opened 4 years ago Updated 3 years ago

crash in mozilla::layers::LayerPropertiesBase::ComputeChange

Categories

(Core :: Graphics: Layers, defect, P3, critical)

Unspecified
All
defect

Tracking

()

REOPENED
mozilla50
Tracking Status
firefox45 --- wontfix
firefox46 --- affected
firefox47 --- affected
firefox48 --- affected
firefox-esr45 --- affected
firefox50 --- affected
firefox51 --- affected
firefox52 --- unaffected

People

(Reporter: milan, Unassigned)

References

Details

(Keywords: crash, Whiteboard: [gfx-noted])

Crash Data

Attachments

(3 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-0b79b518-b8a7-4a9d-ad73-889412160223.
=============================================================

Very low volume crash, but give our OS X population, that multiplies.  It's worth looking at the code and validating some assumptions or dropping a critical error or two if those assumptions not met.
Assignee: nobody → bgirard
Whiteboard: [gfx-noted]
Flags: needinfo?(bgirard)
It's probably coming from this line:
http://hg.mozilla.org/releases/mozilla-release/annotate/60e96806ff1c/gfx/layers/LayerTreeInvalidation.cpp#l154

mLayer must be null. mLayer->GetAncestorMaskLayerCount() would effectively perform:
mLayer->mAncestorMaskLayers.Length()

Only issue with this is 'mAncestorMaskLayers' 7th field of Layer so I'd expect to be crashing at some offset of 0x0 and not exactly at 0x0.
Flags: needinfo?(bgirard)
Here's a better theory: mHdr in the nTArray for mAncestorMaskLayers is 0, thus we crash when trying to perform |mHdr->mLength| (which is mLayer->mAncestorMaskLayers.mHdr->mLength) and since mLength is the first field then we wouldn't increment and would crash at exactly 0x0.

So how can mHdr be null. A quick check reveals that it should be set to sEmptyHdr if it's not set which is definitely not null.
All the Windows crash are at a non null address and all the OSX crashes are at a null address.
In Release this is the #163 crash signature overall but #26 on Mac OS. I'm going to assume this is too late to fix for 45 (likely for 46 as well). Are we any closer to a fix?
Let's at least add some critical errors to see if we can validate some of the thoughts from previous comments.
Flags: needinfo?(bgirard)
It's possible we have a use-after-free/corruption bug. I looked around to see if we had other crashes in the same code and it turns out we do. This supports my theory since corruption could lead us to crash in other fields.

There's also a long tail of crashes in this area as well.
Crash Signature: [@ mozilla::layers::LayerPropertiesBase::ComputeChange] → [@ mozilla::layers::LayerPropertiesBase::ComputeChange] [@ mozilla::layers::ContainerLayerProperties::ComputeChangeInternal]
These are likely related as well. I leave out the longer tail unless their crash report provide additional clues.
Crash Signature: [@ mozilla::layers::LayerPropertiesBase::ComputeChange] [@ mozilla::layers::ContainerLayerProperties::ComputeChangeInternal] → [@ mozilla::layers::LayerPropertiesBase::ComputeChange] [@ mozilla::layers::ContainerLayerProperties::ComputeChangeInternal] [@ nsTArray<T>::~nsTArray | mozilla::layers::LayerPropertiesBase::~LayerPropertiesBase] [@ mozilla::layers::LayerPropertiesBase…
Bug 1268246 would help confirm or rule out use-after-free and corruption.
Depends on: 1268246
Flags: needinfo?(bgirard)
Comment on attachment 8746255 [details]
Bug 1251615 - Add poison values to Layer to check for errors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49333/diff/1-2/
Attachment #8746255 - Attachment description: MozReview Request: Bug 1251615 - WIP Diagnostics → Bug 1251615 - Add poison values to Layer to check for errors.
Attachment #8746255 - Flags: review?(mstange)
Comment on attachment 8746255 [details]
Bug 1251615 - Add poison values to Layer to check for errors.

https://reviewboard.mozilla.org/r/49333/#review62038

::: gfx/layers/Layers.h:1357
(Diff revision 2)
>    FrameMetrics::ViewID GetScrollbarTargetContainerId() { return mScrollbarTargetId; }
>    ScrollDirection GetScrollbarDirection() { return mScrollbarDirection; }
>    float GetScrollbarThumbRatio() { return mScrollbarThumbRatio; }
>    bool IsScrollbarContainer() { return mIsScrollbarContainer; }
>    Layer* GetMaskLayer() const { return mMaskLayer; }
> +  void Check() const { mPoison.Check(); }

Can we call this CheckPoison?
Attachment #8746255 - Flags: review?(mstange) → review+
Comment on attachment 8746255 [details]
Bug 1251615 - Add poison values to Layer to check for errors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49333/diff/2-3/
Yes, I've renamed it to be CheckCanary. I forgot the base patch had changed.
Flags: needinfo?(bgirard)
Pushed by b56girard@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/900b411563be
Add poison values to Layer to check for errors. r=mstange
https://hg.mozilla.org/mozilla-central/rev/900b411563be
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
We're getting closer. Let's check a few more things.
Attachment #8777540 - Flags: review?(mstange)
Attachment #8777540 - Flags: review?(mstange) → review+
This doesn't fix it but just another safety check to disprove that we're slicing the object by accident.
Attachment #8789000 - Flags: review?(mstange)
Comment on attachment 8789000 [details] [diff] [review]
Bug 1251615 - Disallow Copy/Assignment to LayerProperties. r=mstange

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

Sure, why not.

::: gfx/layers/LayerTreeInvalidation.h
@@ +42,3 @@
>    virtual ~LayerProperties() {}
>  
> +

unnecessary additional empty line
Attachment #8789000 - Flags: review?(mstange) → review+
Attachment #8789000 - Attachment is obsolete: true
Flags: needinfo?(bgirard)
Pushed by b56girard@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b931a44e54db
Disallow Copy/Assignment to LayerProperties. r=mstange
Attachment #8789001 - Attachment is patch: true
I've tested just about every wild theory I could come up with this bug, learned a lot of how UniquePtr, rvalues, move semantics.

At this point it's probably best to have a fresh pair of eye on this bug or go for a stronger hammer approach like trying to get a replay.
Assignee: bgirard → nobody
This is still reproducible in low volume on Fx50 (based on the last 2 months).

  SIGNATURE   | mozilla::layers::LayerPropertiesBase::ComputeChange
  -----------------------------------------------------------------
  CRASH STATS | http://tinyurl.com/jrljhjh
  -----------------------------------------------------------------
  OVERVIEW    | 0 crashes on nightly 52
	      | 6 crashes on nightly 51
	      | 0 crashes on aurora 51
	      | 3 crashes on beta 50
  -----------------------------------------------------------------
  LAST CRASH  | 2016-09-25 (on 50.0b1)



  SIGNATURE   | mozilla::layers::ContainerLayerProperties::ComputeChangeInternal
  ------------------------------------------------------------------------------
  CRASH STATS | http://tinyurl.com/zrjgmy4
  ------------------------------------------------------------------------------
  OVERVIEW    | 0 crashes on nightly 52
	      | 1 crash on nightly 51
	      | 0 crashes on aurora 51
	      | 5 crashes on beta 50
  ------------------------------------------------------------------------------
  LAST CRASH  | 2016-09-27 (on 50.0b1)



  SIGNATURE   | nsTArray<T>::~nsTArray | mozilla::layers::LayerPropertiesBase::~LayerPropertiesBase
  -------------------------------------------------------------------------------------------------
  CRASH STATS | http://tinyurl.com/gkrcmrf
  -------------------------------------------------------------------------------------------------
  OVERVIEW    | 0 crashes on nightly 52
	      | 0 crashes on nightly 51
	      | 0 crashes on aurora 51
	      | 0 crashes on beta 50



  SIGNATURE   | mozilla::layers::LayerPropertiesBase::LayerPropertiesBase
  -----------------------------------------------------------------------
  CRASH STATS | http://tinyurl.com/glzy6sh
  -----------------------------------------------------------------------
  OVERVIEW    | 0 crashes on nightly 52
	      | 1 crash on nightly 51
	      | 0 crashes on aurora 51
	      | 3 crashes on beta 50
  -----------------------------------------------------------------------
  LAST CRASH  | 2016-09-28 (on 50.0b2)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 1305375
Crash Signature: mozilla::layers::LayerPropertiesBase::LayerPropertiesBase ] → mozilla::layers::LayerPropertiesBase::LayerPropertiesBase ] [@ mozilla::CorruptionCanary::Check]
OS: Mac OS X → All
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.