961813 - Segfault in StreamTextureHostOGL::GetSize() in layers dump

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

I am getting a segfault in StreamTextureHostOGL::GetSize() when trying to do a layers dump on B2G.

Benoit Jacob and I investigated this a bit. The problem is that TextureHost::PrintInfo() unconditionally calls TextureHost::GetSize(). StreamTextureHostOGL::GetSize() in turn calls mTextureSource->GetSize(), but mTextureSource is sometimes null.

It seems that mTextureSource being null is legitimate, as StreamTextureHostOGL only creates the texture source in Lock(). Therefore, PrintInfo() needs to guard against this case and not call GetSize() in such a case (GetFormat() has the same problem).

The problem is, PrintInfo() is implemented in the base class TextureHost, which doesn't have the information of whether it's safe to call GetSize() and GetFormat() (e.g. in the case of a StreamTextureHostOGL, whether its mTextureSource is null).

Some possible ways of resolving this problem are to reimplement PrintInfo() in the derived class StreamTextureHostOGL and add the guard there (but this is an unfortunate duplication of code), or to add a virtual method like IsValid() to TextureHost which would return false if it is not safe to call things like GetSize() and GetFormat() (but this is introducing a new concept to the base class and maybe we can avoid that).

I am cc'ing a few people Benoit suggested might have knowledge of this code and might be able to suggest a better solution.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Adding ni?nical since this is his API :)

I think you need to Lock() the texture before you can safely call GetSize/Format, but we should make a decision here and document it clearly. And probably add asserts to ensure that the texture is locked when GetSize is called.

Alternatively we could not require this, and change the implementation of GetSize() to be safe when the texture isn't locked.

Comment 2

[Nicolas Silva [:nical]]

All the other TextureHost implementations store their size and format to avoid having to lock and access their shared data when dumping things. Otherwise you can Lock the texture as matt proposed. I don't feel too strongly either way since dumping layers is just a debugging tool.

Comment 3

[Botond Ballo [:botond]]

Attachment: bug961813.patch

The attached patch locks the surface before calling GetSize() and GetFormat() in PrintInfo().

Try run: https://tbpl.mozilla.org/?tree=Try&rev=3f5c855c1a4d

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8363213 [details] [diff] [review]
bug961813.patch

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

r=me with the comment below fixed

::: gfx/layers/composite/TextureHost.cpp
@@ +245,5 @@
>    aTo += aPrefix;
>    aTo += nsPrintfCString("%s (0x%p)", Name(), this);
> +  // Note: the TextureHost needs to be locked before it is safe to call
> +  //       GetSize() and GetFormat() on it.
> +  Lock();

It'd be even better if you check the result of Lock
if (Lock()) {
  // print stuff
  Unlock();
}

Comment 5

[Botond Ballo [:botond]]

Attachment: bug961813.patch

Updated as per review comment. Carrying r+.

Comment 6

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/343839a83b38

Comment 7

[Kartikaya Gupta (email:[e-mail])]

This landed on central:
https://hg.mozilla.org/mozilla-central/rev/343839a83b38

but there was a typo in the bug number in the commit message.