Closed Bug 912173 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

All
Android
defect
Not set
critical

Tracking

(firefox25- fixed, firefox26- fixed)

RESOLVED FIXED
Firefox 26
Tracking Status
firefox25 - fixed
firefox26 - fixed

People

(Reporter: jchen, Assigned: mattwoodrow)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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)
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.
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)
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 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+
https://hg.mozilla.org/mozilla-central/rev/353aa004fed8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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?
Attachment #801324 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.