Closed Bug 959615 Opened 7 years ago Closed 7 years ago

crash in mozilla::layers::CompositableParentManager::ReceiveCompositableUpdate(mozilla::layers::CompositableOperation const&, std::vector<mozilla::layers::EditReply, std::allocator<mozilla::layers::EditReply> >&)


(Core :: Graphics: Layers, defect)

27 Branch
Not set



Tracking Status
firefox26 --- wontfix
firefox27 + wontfix
firefox28 + verified
firefox29 --- verified


(Reporter: tracy, Assigned: nical)


(Keywords: crash, regression, Whiteboard: [testday-20140214])

Crash Data


(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-8eb445a6-570a-4c2e-804b-dd9a92140114.

Filing this as a desktop follow up to bug 923917 which apparently only fixed mobile.  It is #1 topcrasher for Mac on 27betas
Flags: needinfo?(nical.bugzilla)
requesting tracking as it is a #1 platform crasher (Mac).
Assignee: nobody → nical.bugzilla
TextureParent::mTextureHost was left uninitialized in the constructor, which is bad because it made us not catch the problem with the assertion when CompositableTransactionParent.cpp is handling the OpUpdateTexture oeration.

Now we have to figure out how we managed to receive a transaction referring to a PTexture before the latter received it's Init message.
Attachment #8360373 - Flags: review?(bjacob)
Flags: needinfo?(nical.bugzilla)
I don't understand how that is possible, given that mTextureHost is a RefPtr, which has a constructor initializing the pointer to null?
ah that's true :(
Attachment #8360373 - Flags: review?(bjacob)
Currently we first create the PTexture IDPL pair and then initialize it in a second message: async Init(SurfaceDescriptor, Flags)

We don't need to do it in two steps, and this bug seems to indicate that we are using a TextureParent that hasn't yet received its initialization.

So this patch simplifies things by initializing the TextureParent in it's construction phase rather than after receiving an Init message.
Attachment #8360373 - Attachment is obsolete: true
Attachment #8362497 - Flags: review?(bjacob)
Comment on attachment 8362497 [details] [diff] [review]
Initialize the TextureChild/parent pair at creation time instead of in a separate message

Review of attachment 8362497 [details] [diff] [review]:

::: gfx/layers/client/TextureClient.cpp
@@ +174,4 @@
>    mActor->mForwarder = aForwarder;
>    mActor->mTextureClient = this;
>    mShared = true;
> +  return true;

Do keep that IPCOpen() check (and make sure you're keeping them elsewhere too).

::: gfx/layers/composite/TextureHost.cpp
@@ +59,3 @@
>  {
> +  TextureParent* actor = new TextureParent(aAllocator);
> +  DebugOnly<bool> status = actor->Init(aSharedData, aFlags);

It would be nice if you could merge that Init() method into the constructor in a follow-up patch, to mirror the change you just made at the protocol level.
Attachment #8362497 - Flags: review?(bjacob) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
:nical, if this is low risk enough to uplift on Fx27, please request uplift with risk analysis so this can get in our beta going to build tomorrow if appropriate given this is called #1 platform crasher on mac.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8362497 [details] [diff] [review]
Initialize the TextureChild/parent pair at creation time instead of in a separate message

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 897452
User impact if declined: crashes
Testing completed (on m-c, etc.): only a day :(
Risk to taking this patch (and alternatives if risky): low risk, worse case the patch doesn't fix entirely the crashes (it hasn't been baking in m-c long enough to confirm everything is fixed).
String or IDL/UUID changes made by this patch:
Attachment #8362497 - Flags: approval-mozilla-aurora?
Flags: needinfo?(nical.bugzilla)
Attachment #8362497 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This needs a branch-specific patch for the Aurora uplift.
Wait, actually, this patch fixes a regression from bug 897452 which hasn't made it into aurora yet, so there is nothing to uplift.
Target Milestone: mozilla29 → ---
How can it be a regression from bug 897452 is it's a topcrash on beta27?
Flags: needinfo?(nical.bugzilla)
Target Milestone: --- → mozilla29
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #13)
> How can it be a regression from bug 897452 is it's a topcrash on beta27?

There must have been a confusion on my side and two separate bugs with very similar signatures before and after PTexture landed (which affected a lot the code where we crash now). I need to catch a train right now but I can make an emergency patch this weekend that does an early return without crashing where the bug is happening (in aurora) when we get into the invalid state. We might miss a frame but at least we won't crash.
Flags: needinfo?(nical.bugzilla)
We're past the time for speculative fixes on beta, so this is going to be a wontfix there but still tracking for potential forward fix if low-risk enough to take to FF28
This patch forces us to return early if the texture is null in release builds (keeping the fatal assertion for debug builds). We'll probably drop a frame in this scenario but it's better than crashing.
Attachment #8365850 - Flags: review?(bjacob)
Resolution: FIXED → ---
Attachment #8365850 - Flags: review?(bjacob) → review+

FWIW, this probably shouldn't have been reopened just for a branch uplift. We typically use the bug resolution to track the status on trunk.
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Keywords: verifyme
I'm not seeing any crashes for this on Firefox 28.0b, but there is a single crash for 28.0a:
Whiteboard: [testday-20140214]
You need to log in before you can comment on or make changes to this bug.