Closed Bug 916714 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Android
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: jesup, Assigned: bjacob)

References

Details

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

Attachments

(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 <bjacob@mozilla.com>
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?
https://hg.mozilla.org/mozilla-central/rev/55afb38c243a
Status: NEW → RESOLVED
Closed: 6 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.