Closed Bug 916714 Opened 8 years ago Closed 8 years ago

Assertion "Will leak the old mBuffer" when trying to capture video on Android


(Core :: WebRTC: Audio/Video, defect)

Not set



Tracking Status
firefox26 --- fixed
firefox27 --- fixed


(Reporter: jesup, Assigned: bjacob)



(Keywords: assertion, regression, Whiteboard: [getusermedia])


(2 files)

Attached file callstack
Recent regression on 26 when trying to capture video in mozilla::layers::DeprecatedTextureHost::SetBuffer() at ../../dist/include/mozilla/layers/TextureHost.h:712
712         MOZ_ASSERT(!mBuffer, "Will leak the old mBuffer");
The first bad revision is:
changeset:   146641:e20f0978e31e
user:        Benoit Jacob <>
date:        Wed Sep 11 13:39:04 2013 -0400
summary:     Bug 912725 - Do the registration of the TextureHost with the mBuffer exactly when we overwrite *mBuffer, so that in particular we don't do a bogus registration in the single-buffered case - r=nical
Thanks for the bisection. Looking.
Assignee: gpascutto → bjacob
Wait, we use gralloc on *android* (not just on B2G) ?
Flags: needinfo?(snorp)
Note sure if snorp is available this week. Blassey?
Severity: normal → major
Flags: needinfo?(blassey.bugs)
Version: 22 Branch → Trunk
Benoit, any thoughts here?
Flags: needinfo?(blassey.bugs)
Right, so, here is the story.

TLDR: nothing to see here, no gralloc, and no leak. Just a bad assertion.

This assertion is in a base class method. While the bug that regressed it was b2g-specific and gralloc-specific, the assertion here is in non-b2g-specific, non-gralloc-specific code.

This assertion is also wrong:

  virtual void SetBuffer(SurfaceDescriptor* aBuffer, ISurfaceAllocator* aAllocator)
    MOZ_ASSERT(!mBuffer, "Will leak the old mBuffer");
    mBuffer = aBuffer;
    mDeAllocator = aAllocator;

Here we are saying that if mBuffer previously pointed to something, then assigning something to it will cause the old thing to be leaked.

But there is a case there that isn't true: the case where mBuffer == aBuffer, in which case this assignment does nothing!

And that's exactly what we have started doing explicitly in the patch that regressed this. Why do this, if it's a no-operation? Because SetBuffer is virtual, and is overridden in the b2g-specific gralloc path, where that becomes a useful operation even when mBuffer==aBuffer.
Attachment #806061 - Flags: review?(jmuizelaar)
No gralloc here, cancelling the needinfo.
Flags: needinfo?(snorp)
Attachment #806061 - Flags: review?(jmuizelaar) → review+
Comment on attachment 806061 [details] [diff] [review]
Fix the assertion on mBuffer

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 912725
User impact if declined: assertion (crashes debug builds)
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #806061 - Flags: approval-mozilla-aurora?
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #806061 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.