If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

crash in mozilla::RefPtr<mozilla::gl::GLContext>::~RefPtr()

RESOLVED FIXED in Firefox 25

Status

()

Firefox for Android
Graphics, Panning and Zooming
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jchen, Assigned: mattwoodrow)

Tracking

({crash})

Trunk
Firefox 26
All
Android
crash
Points:
---

Firefox Tracking Flags

(firefox25- fixed, firefox26- fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
This bug was filed from the Socorro interface and is 
report bp-6f45a537-55b9-4a7f-a342-54cc72130901.
=============================================================

Very frequent when trying the pdf.js add-on in Fennec.
Seems like a bug around SurfaceStreamHostOGL which is used to composite Skia/GL 2D canvases on Linux.

Not clear to me who should look into this bug, let's ask milan. Skia/GL people would be :gw280 or :snorp or me, while Layers people would be :nical or Bas or :nrc or me, I guess. Basically, anyone in the gfx team is a reasonable choice.
Flags: needinfo?(milan)
:snorp is on vacation this week, George could take a look - do we have any crash stats on this?
Flags: needinfo?(milan)
(Reporter)

Comment 3

4 years ago
It's the #9 crasher on Fennec Nightly but the number of crashes is not a lot. It does happen very frequently when testing pdf.js on Fennec.

Since I've been looking at pdf.js stuff, I will go ahead and debug this and see if I can find anything. If not, I will bug Snorp or George about it later.
(Reporter)

Comment 4

4 years ago
The crash happens when attempting to release an already destroyed SurfaceStreamHostOGL::mStreamGL, which was added in bug 894405. The comments in bug 894405 kind of predicted this regression :(

I think this is a regression from bug 904620. That bug took out the mNeedsUpdate flag which means GLContext::AddRef [1] is only called for some updates, whereas GLContext::Release is called for all updates on the other side [2], causing a mismatch.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/CanvasClient.cpp#218
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp#648

I'm not sure what's the correct way to fix it. Can you take this, Matt?
Blocks: 904620
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 5

4 years ago
Created attachment 801324 [details] [diff] [review]
Don't try to minimize the number of AddRef/Release pairs

Thanks for debugging this Jim.

It looks like we're recreating the TextureHost object most times, so we're releasing the GLContext more than we AddRef it.

This is a known bug that should be fixed by new textures, so I'll leave it for now.

Even with that fixed, it's still at least valid for the host side to recreate textures, so I don't think we can have these checks work reliably.

The cost of a few extra refs should be fine.
Attachment #801324 - Flags: review?(ncameron)
Flags: needinfo?(matt.woodrow)

Comment 6

4 years ago
Comment on attachment 801324 [details] [diff] [review]
Don't try to minimize the number of AddRef/Release pairs

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

::: gfx/layers/client/CanvasClient.cpp
@@ +206,5 @@
>  #endif
>    } else {
>      SurfaceStreamHandle handle = stream->GetShareHandle();
>      SurfaceDescriptor *desc = mDeprecatedTextureClient->GetDescriptor();
> +    *desc = SurfaceStreamDescriptor(handle, false);

This is an ugly as hell way to set the descriptor on the texture client. Is there a SetDescriptor we can use?
Attachment #801324 - Flags: review?(ncameron) → review+
(Assignee)

Updated

4 years ago
Duplicate of this bug: 911680
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/353aa004fed8

Comment 9

4 years ago
https://hg.mozilla.org/mozilla-central/rev/353aa004fed8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26

Updated

4 years ago
Duplicate of this bug: 912678
(Assignee)

Comment 11

4 years ago
Comment on attachment 801324 [details] [diff] [review]
Don't try to minimize the number of AddRef/Release pairs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 904620
User impact if declined: Can't uplift 904620
Testing completed (on m-c, etc.): Been on m-c/aurora.
Risk to taking this patch (and alternatives if risky): Very low risk.
String or IDL/UUID changes made by this patch: None
Attachment #801324 - Flags: approval-mozilla-beta?
status-firefox25: --- → affected
status-firefox26: --- → fixed
tracking-firefox25: --- → ?
tracking-firefox26: --- → ?

Updated

4 years ago
Attachment #801324 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

4 years ago
tracking-firefox25: ? → -
tracking-firefox26: ? → -
https://hg.mozilla.org/releases/mozilla-beta/rev/86561d4d413f
Assignee: nobody → matt.woodrow
status-firefox25: affected → fixed
You need to log in before you can comment on or make changes to this bug.