3.92 KB, patch
|Details | Diff | Splinter Review|
4.17 KB, patch
|Details | Diff | Splinter Review|
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.
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.
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozilla::layers::Layer::SetVisibleRegion]
Component: Untriaged → Graphics: Layers
Ever confirmed: true
OS: Windows 10 → All
Product: Firefox → Core
Hardware: Unspecified → All
Almost all crashes happened under LayerTransactionParent::RecvUpdate(). From it, LayerManagerComposite might be destroyed already.
When the crash happens, Layer should be still alive since ShadowLayerParent holds a ref count of the Layer.
(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.
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.
Attachment #8804189 - Attachment is obsolete: false
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  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?).  https://crash-stats.mozilla.com/report/index/360860c3-8d06-42c8-867d-f439d2161020
(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.
Any progress on this, Sotaro?
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.
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.
:mattwoodrow, how about attachment 8823984 [details] [diff] [review]? It seems enough to address the crash.
Can we just update the pointer on the underlying Layer instead?
(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.
I rechecked the source, Layer::mManager is updated manually like  except RefLayerComposite.  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/PaintedLayerComposite.cpp#83
We also need to ensure LayerManager's lifetime by modifying CompositorBridgeParent::StopAndClearResources().
Comment on attachment 8824287 [details] [diff] [review] patch - Update LayerManager correctly carry = :mattwoodrow.
Attachment #8824287 - Flags: review+
attachment 8824287 [details] [diff] [review] caused tryserver tests failure, I am going to address it.
Attachment #8824286 - Attachment is obsolete: false
Attachment #8824287 - Attachment is obsolete: true
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
Attachment #8825308 - Flags: review?(matt.woodrow)
Attachment #8825308 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8825308 - Flags: sec-approval?
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.
Attachment #8825308 - Flags: sec-approval? → sec-approval+
Hi :sotaro, Is this bug worth uplifting to Beta51 & Aurora52 given it's a sec-high?
Patch for aurora.
Attachment #8826432 - Flags: review+
Attachment #8826432 - Attachment description: patch for aurora - Update LayerManager pointer of all Layers of LayerTransactionParent → patch for aurora and beta - Update LayerManager pointer of all Layers of LayerTransactionParent
(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 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 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.
QA Contact: kjozwiak
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
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?
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.
You need to log in before you can comment on or make changes to this bug.