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

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Tracy Walker [:tracy]]

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

Comment 1

[Tracy Walker [:tracy]]

requesting tracking as it is a #1 platform crasher (Mac).

Comment 2

[Nicolas Silva [:nical]]

Attachment: 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.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

I don't understand how that is possible, given that mTextureHost is a RefPtr, which has a constructor initializing the pointer to null?

Comment 4

[Nicolas Silva [:nical]]

ah that's true :(

Comment 5

[Nicolas Silva [:nical]]

Attachment: 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.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 7

[Nicolas Silva [:nical]]

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

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[bhavana bajaj [:bajaj]]

: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.

Comment 10

[Nicolas Silva [:nical]]

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:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This needs a branch-specific patch for the Aurora uplift.

Comment 12

[Nicolas Silva [:nical]]

Wait, actually, this patch fixes a regression from bug 897452 which hasn't made it into aurora yet, so there is nothing to uplift.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

How can it be a regression from bug 897452 is it's a topcrash on beta27?

Comment 14

[Nicolas Silva [:nical]]

(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.

Comment 15

[bhavana bajaj [:bajaj]]

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

Comment 16

[Nicolas Silva [:nical]]

Attachment: 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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 18

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

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

Comment 19

[Ioana (away)]

I'm not seeing any crashes in Socorro for Firefox 28 Beta and 29 Aurora, but there are 7 crashes for Firefox 30 Nightly:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Alayers%3A%3ACompositableParentManager%3A%3AReceiveCompositableUpdate%28mozilla%3A%3Alayers%3A%3ACompositableOperation+const%26%2C+std%3A%3Avector%3Cmozilla%3A%3Alayers%3A%3AEditReply%2C+std%3A%3Aallocator%3Cmozilla%3A%3Alayers%3A%3AEditReply%3E+%3E%26%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A30.0a1&version=Firefox%3A29.0a2&version=Firefox%3A28.0b&version=Firefox%3A28.0b4&version=Firefox%3A28.0b3&version=Firefox%3A28.0b2&version=Firefox%3A28.0b1&hang_type=any&date=2014-02-19+12%3A00%3A00&range_value=4#tab-reports

All the crashes happened during February 4th to 6th.