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

VERIFIED FIXED in Firefox 28

Status

()

Core
Graphics: Layers
--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: tracy, Assigned: nical)

Tracking

({crash, regression})

27 Branch
mozilla29
All
Mac OS X
crash, regression
Points:
---

Firefox Tracking Flags

(firefox26 wontfix, firefox27+ wontfix, firefox28+ verified, firefox29 verified)

Details

(Whiteboard: [testday-20140214], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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

Updated

4 years ago
Flags: needinfo?(nical.bugzilla)
(Reporter)

Comment 1

4 years ago
requesting tracking as it is a #1 platform crasher (Mac).
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
tracking-firefox27: --- → ?

Updated

4 years ago
tracking-firefox27: ? → +
tracking-firefox28: --- → +
Assignee: nobody → nical.bugzilla
(Assignee)

Comment 2

4 years ago
Created attachment 8360373 [details] [diff] [review]
Fix uninitialized member in TextureParent

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?
(Assignee)

Comment 4

4 years ago
ah that's true :(
Attachment #8360373 - Flags: review?(bjacob)
(Assignee)

Comment 5

4 years ago
Created attachment 8362497 [details] [diff] [review]
Initialize the TextureChild/parent pair at creation time instead of in a separate message

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+
https://hg.mozilla.org/mozilla-central/rev/a10cce12e282
Status: NEW → RESOLVED
Last Resolved: 4 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)
(Assignee)

Comment 10

4 years ago
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)

Updated

4 years ago
Attachment #8362497 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This needs a branch-specific patch for the Aurora uplift.
status-firefox26: affected → wontfix
status-firefox29: affected → fixed
Keywords: branch-patch-needed
(Assignee)

Comment 12

4 years ago
Wait, actually, this patch fixes a regression from bug 897452 which hasn't made it into aurora yet, so there is nothing to uplift.
status-firefox26: wontfix → affected
status-firefox29: fixed → affected
Target Milestone: mozilla29 → ---
How can it be a regression from bug 897452 is it's a topcrash on beta27?
status-firefox26: affected → wontfix
status-firefox29: affected → fixed
Flags: needinfo?(nical.bugzilla)
Target Milestone: --- → mozilla29
(Assignee)

Comment 14

4 years ago
(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
status-firefox27: affected → wontfix
(Assignee)

Comment 16

4 years ago
Created attachment 8365850 [details] [diff] [review]
Early return instead of crash if we get into an unexpected state in ReceiveCompositableUpdate

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)
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8365850 - Flags: review?(bjacob) → review+
https://hg.mozilla.org/releases/mozilla-aurora/rev/07f77213ecf1

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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-firefox28: affected → fixed
Keywords: branch-patch-needed
Resolution: --- → FIXED

Updated

4 years ago
Keywords: verifyme
I'm not seeing any crashes for this on Firefox 28.0b, but there is a single crash for 28.0a: https://crash-stats.mozilla.com/report/index/4044728a-91f8-4e38-bce6-d28932140214
Whiteboard: [testday-20140214]
You need to log in before you can comment on or make changes to this bug.