1307458 - crash in mozilla::layers::Layer::SetVisibleRegion

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Esther Monchari]

This bug was filed from the Socorro interface and its 
report https://crash-stats.mozilla.com/report/index/7cd7f98f-9ac0-40b0-b462-20a7f2160922

This was noted while looking at nightly crash stats.As far as I found out the crash associated multiple users. The crash started on 2016-09-21 23:19:11  with build id 20160920030429 and is still continuing.

The crash is mostly on firefox 52.0a1 version ,windows 10 OS

need info on Carsten since it appears he worked in that area and can maybe help figure out what could be causing the crash.

Comment 1

[Marcia Knous [:marcia]]

Adding crash signature field, and switching over to what I believe to be the correct component. There is also one Mac crash so changing Platform info.

adding ni on :mattwoodrow in case he has some ideas.

Comment 3

[Sotaro Ikeda [:sotaro]]

Almost all crashes happened under LayerTransactionParent::RecvUpdate(). From it, LayerManagerComposite might be destroyed already.

Comment 4

[Sotaro Ikeda [:sotaro]]

When the crash happens, Layer should be still alive since ShadowLayerParent holds a ref count of the Layer.

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Almost all crashes happened under LayerTransactionParent::RecvUpdate(). From
> it, LayerManagerComposite might be destroyed already.

At first LayerManagerComposite of the Layer should be always valid since LayerTransactionParent holds a ref count of the LayerManagerComposite.

The Layer's reference of LayerManagerComposite is updated by CompositorBridgeParent::RecvAdoptChild(). It seems related to the crash.

Comment 6

[Sotaro Ikeda [:sotaro]]

If LayerTransactionParent and Layer holds same ref to LayerManagerComposite, the crash should not happen. Then it seems to mean that they hold different refs of LayerManagerComposite.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Check LayerManager mismatch

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Check LayerManager mismatch

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Check LayerManager mismatch

Comment 10

[Matt Woodrow (:mattwoodrow)]

Layer::mManager is a raw pointer, so if trying to access it is causing a crash, then we have a real problem.

From looking at the reports, it appears that at least in some cases [1] mManager is non-null, so we're trying to access a dangling pointer.

That sounds like a real security bug, so just early returning here probably isn't sufficient.

We really need to make sure LayerManagers outlive all their child layers (and maybe come up with a safer way to ensure this doesn't regress?).



[1] https://crash-stats.mozilla.com/report/index/360860c3-8d06-42c8-867d-f439d2161020

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> 
> We really need to make sure LayerManagers outlive all their child layers
> (and maybe come up with a safer way to ensure this doesn't regress?).
> 

Thanks for the review. I am going to try to address the comment.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Any progress on this, Sotaro?

Comment 13

[Matt Woodrow (:mattwoodrow)]

I think the right thing to do here is to make all the raw Layer*/LayerManager* members hold a strong ref using RefPtr<>, and then make sure we always break any cycles explicitly.

A few I can think of right now:

LayerManager::mRoot
Layer::mManager
Layer::mNextSibling
ContainerLayer::mFirstChild.

Comment 14

[Sotaro Ikeda [:sotaro]]

From the source, direct cause of the crash might be related to the following.

CompositorBridgeParent::RecvAdoptChild() updates HostLayerManager of Layers and LayerTransactionParent. but it does not update a pointer of LayerManager of Layers. Then the pointer of LayerManager continue to refer to old HostLayerManager.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - make LayerManager of LayerComposite alive

Comment 16

[Sotaro Ikeda [:sotaro]]

:mattwoodrow, how about attachment 8823984 [details] [diff] [review]? It seems enough to address the crash.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Can we just update the pointer on the underlying Layer instead?

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Can we just update the pointer on the underlying Layer instead?

I am going to try it.

Comment 19

[Sotaro Ikeda [:sotaro]]

I rechecked the source, Layer::mManager is updated manually like [1] except RefLayerComposite.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/PaintedLayerComposite.cpp#83

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add RefLayerComposite::SetLayerManager()

Comment 21

[Sotaro Ikeda [:sotaro]]

We also need to ensure LayerManager's lifetime by modifying CompositorBridgeParent::StopAndClearResources().

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update LayerManager correctly

Comment 23

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8824287 [details] [diff] [review]
patch - Update LayerManager correctly

carry = :mattwoodrow.

Comment 24

[Sotaro Ikeda [:sotaro]]

attachment 8824287 [details] [diff] [review] caused tryserver tests failure, I am going to address it.

Comment 25

[Sotaro Ikeda [:sotaro]]

There might be a case that the following in CompositorBridgeParent::RecvAdoptChild() could not update LayerManager pointer of all Layers of LayerTransactionParent if there is a Layer that does not exist under the mRoot;

> sIndirectLayerTrees[child].mRoot->AsHostLayer()->SetLayerManager(static_cast<HostLayerManager*>(mLayerManager.get()));

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp#1527

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update LayerManager pointer of all Layers of LayerTransactionParent

Comment 27

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d8ceac83be63f8cce3981963dc3b4fd0ca4c614

Comment 28

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8825308 [details] [diff] [review]
patch - Update LayerManager pointer of all Layers of LayerTransactionParent

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

 It seems difficult to create the exploit based on this patch.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

  No. The patch does not have a comment of saying that the patch fixes a security problem.

> Which older supported branches are affected by this flaw?

It causes the problem when e10s is enabled. e10s seems to be enabled since Firefox 48.

> If not all supported branches, which bug introduced the flaw?

  Bug 918634 introduced the flaw. It caused the problem only when e10s is enabled.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

  No yet. The patch depends on Bug 1323539. A patch becomes a bit different. Creating a patch for the branches is not difficult.

> How likely is this patch to cause regressions; how much testing does it need?

  This fix is low risk of regression.
  I tested locally and tryserver test passed.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8825308 [details] [diff] [review]
patch - Update LayerManager pointer of all Layers of LayerTransactionParent

sec-approval+ for trunk.
we'll want patches nominated for Beta and Aurora as soon as possible so we can get it in the final beta that's going to be made real soon now.

Comment 30

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e876cfcd849281493e4fa1f71789a010b6700

Comment 31

[Gerry Chang [:gchang]]

Hi :sotaro,
Is this bug worth uplifting to Beta51 & Aurora52 given it's a sec-high?

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch for aurora and beta - Update LayerManager pointer of all Layers of LayerTransactionParent

Patch for aurora.

Comment 33

[Sotaro Ikeda [:sotaro]]

(In reply to Gerry Chang [:gchang] from comment #31)
> Hi :sotaro,
> Is this bug worth uplifting to Beta51 & Aurora52 given it's a sec-high?

Yes. It worth uplifting.

Comment 34

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8826432 [details] [diff] [review]
patch for aurora and beta - Update LayerManager pointer of all Layers of LayerTransactionParent

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Moving tab between windows could cause crash.
[Is this code covered by automated tests?]: The touched code is functionally tested.
[Has the fix been verified in Nightly?]: Not yet. The patch is in inbound now.
[Needs manual test from QE? If yes, steps to reproduce]: No, we do not know  the explicit STR to reproduce the crash.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]:It actually does not change how LayerTransactionParent works in normal situation.
[String changes made/needed]: none

Comment 35

[Carsten Book [:Tomcat]]

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

Comment 36

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

Comment on attachment 8826432 [details] [diff] [review]
patch for aurora and beta - Update LayerManager pointer of all Layers of LayerTransactionParent

Fix for sec-high issue, let's try to get this into Monday's 51RC buld.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/295f6ec43e36
https://hg.mozilla.org/releases/mozilla-beta/rev/9b01b6d9151d

Comment 38

[Gerry Chang [:gchang]]

Track 51+ as sec-high.

Comment 39

[Kamil Jozwiak [:kjozwiak]]

This is going to be hard to verify as there's no STR nor any attached test cases as mentioned in comment#34. However, looking at the signature under crash stats, it seems like this has been fixed. I'm not seeing any crashes from 51RC1 or 51RC2 which were created after the patch landed.

Liz, do we usually have enough people using RC1/RC2 builds that we can confidently say that this has beed fixed without actually testing it ourselves?

Comment 40

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

Most of the beta 51 population is currently on the RC build right now. I see a few crashes for beta 14 but not for the RC builds, so that's a good sign.