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

Status: RESOLVED FIXED

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

Description

[Randell Jesup [:jesup] (needinfo me)]

Attachment: 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");

Comment 1

[Gian-Carlo Pascutto [:gcp]]

The first bad revision is:
changeset:   146641:e20f0978e31e
user:        Benoit Jacob <[e-mail]>
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

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for the bisection. Looking.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Wait, we use gralloc on *android* (not just on B2G) ?

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Note sure if snorp is available this week. Blassey?

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Benoit, any thoughts here?

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix the assertion on mBuffer

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.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

No gralloc here, cancelling the needinfo.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55afb38c243a

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55afb38c243a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/04a42db395f2