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

RESOLVED FIXED in Firefox 38

Status

()

Core
Graphics: Layers
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mad_maks, Assigned: nical)

Tracking

({crash})

unspecified
mozilla40
x86
Windows NT
crash
Points:
---

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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
(Reporter)

Comment 1

3 years ago
Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Firefox/38.0
Caused by bug 1036785?
(Assignee)

Updated

3 years ago
Assignee: nobody → nical.bugzilla
(Assignee)

Comment 3

3 years ago
Created attachment 8582461 [details] [diff] [review]
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.
Attachment #8582461 - Flags: review?(jmuizelaar)
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?
Attachment #8582461 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 5

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
Created attachment 8583844 [details] [diff] [review]
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.
Attachment #8582461 - Attachment is obsolete: true
Attachment #8583844 - Flags: review?(jmuizelaar)
(Assignee)

Comment 7

3 years ago
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
Attachment #8583844 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/3d625a4d0935
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Nicolas, can this patch be uplifted to Fx38? Hello screensharing is available there, so crashes will happen there too - see bug 1153638.
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 11

3 years ago
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]:
Flags: needinfo?(nical.bugzilla)
Attachment #8583844 - Flags: approval-mozilla-beta?
status-firefox38: --- → affected
status-firefox39: --- → ?
Comment on attachment 8583844 [details] [diff] [review]
null check without warning.

[Triage Comment]

I think we want it in 39 too (aurora currently)
Attachment #8583844 - Flags: approval-mozilla-beta?
Attachment #8583844 - Flags: approval-mozilla-beta+
Attachment #8583844 - Flags: approval-mozilla-aurora+
status-firefox39: ? → affected
Duplicate of this bug: 1153638
You need to log in before you can comment on or make changes to this bug.