1145981 - crash in mozilla::layers::DIBTextureHost::Updated(nsIntRegion const*)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Tim Maks van den Broek [:mad_maks]]

steps to reproduce:
- Firefox Hello
- connect to someone
- share tabs

This bug was filed from the Socorro interface and is 
report bp-1030650e-a851-4a22-a28c-fdd402150321.
=============================================================
0 	xul.dll 	mozilla::layers::DIBTextureHost::Updated(nsIntRegion const*) 	gfx/layers/TextureDIB.cpp
1 	xul.dll 	mozilla::layers::CompositableParentManager::ReceiveCompositableUpdate(mozilla::layers::CompositableOperation const&, std::vector<mozilla::layers::EditReply, std::allocator<mozilla::layers::EditReply> >&) 	gfx/layers/ipc/CompositableTransactionParent.cpp
2 	xul.dll 	mozilla::layers::ImageBridgeParent::RecvUpdate(nsTArray<mozilla::layers::CompositableOperation>&&, nsTArray<mozilla::layers::EditReply>*) 	gfx/layers/ipc/ImageBridgeParen

Comment 1

[Tim Maks van den Broek [:mad_maks]]

Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Firefox/38.0

Comment 2

[Martijn Wargers (dead)]

Caused by bug 1036785?

Comment 3

[Nicolas Silva [:nical]]

Attachment: null-check

I have another (more invlolved) patch in the making that tries to address the reason why we get into this situation where we don't have a compositor.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8582461 [details] [diff] [review]
null-check

Review of attachment 8582461 [details] [diff] [review]:
-----------------------------------------------------------------

Can you

::: gfx/layers/TextureDIB.cpp
@@ +146,5 @@
>  {
> +  if (!mCompositor) {
> +    NS_WARNING("Ǹeed a valid Compositor to update a DIB TextureHost.");
> +    return;
> +  }

Please add a comment about what situations cause us to not have a mCompositor and how we recover from this?. Do other texture hosts need to be updated to handle the no mCompositor case?

Comment 5

[Nicolas Silva [:nical]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Comment on attachment 8582461 [details] [diff] [review]
> null-check
> 
> Review of attachment 8582461 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you
> 
> ::: gfx/layers/TextureDIB.cpp
> @@ +146,5 @@
> >  {
> > +  if (!mCompositor) {
> > +    NS_WARNING("Ǹeed a valid Compositor to update a DIB TextureHost.");
> > +    return;
> > +  }
> 
> Please add a comment about what situations cause us to not have a
> mCompositor and how we recover from this?

This is currently in flux, because of the other patch I am working on.

>. Do other texture hosts need to be
> updated to handle the no mCompositor case?

Most of them already check for mCompositor and return with a warning in this situation.

Comment 6

[Nicolas Silva [:nical]]

Attachment: null check without warning.

With bug 1147894 we are as close as it gets to be confident that if mCompositor is null, it means that the Compositable isn't yet attached to a layer, in which case it is fine to skip the upload.

Comment 7

[Nicolas Silva [:nical]]

Got mixed up, landed this instead of another patch, and backed it out right after (not reviewed yet).

https://hg.mozilla.org/integration/mozilla-inbound/rev/48a6591e10a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e0e159457ec

Comment 8

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d625a4d0935

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d625a4d0935

Comment 10

[Mike de Boer [:mikedeboer]]

Nicolas, can this patch be uplifted to Fx38? Hello screensharing is available there, so crashes will happen there too - see bug 1153638.

Comment 11

[Nicolas Silva [:nical]]

Comment on attachment 8583844 [details] [diff] [review]
null check without warning.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: crashes when using hello in a certain way.
[Describe test coverage new/current, TreeHerder]: none.
[Risks and why]: low, just a trivial null check preventing us from choking on a null pointer in a place where it's better to skip work than crash on it. Other backends already do that null check.
[String/UUID change made/needed]:

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8583844 [details] [diff] [review]
null check without warning.

[Triage Comment]

I think we want it in 39 too (aurora currently)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/52ac172fbfe1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/16d7e20d9565