Closed Bug 961813 Opened 9 years ago Closed 9 years ago

Segfault in StreamTextureHostOGL::GetSize() in layers dump


(Core :: Graphics: Layers, defect)

Gonk (Firefox OS)
Not set





(Reporter: botond, Assigned: botond)



(1 file, 1 obsolete file)

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.
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.
Flags: needinfo?(nical.bugzilla)
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.
Flags: needinfo?(nical.bugzilla)
Attached patch bug961813.patch (obsolete) — Splinter Review
The attached patch locks the surface before calling GetSize() and GetFormat() in PrintInfo().

Try run:
Assignee: nobody → botond
Attachment #8363213 - Flags: review?(nical.bugzilla)
Comment on attachment 8363213 [details] [diff] [review]

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
Attachment #8363213 - Flags: review?(nical.bugzilla) → review+
Attached patch bug961813.patchSplinter Review
Updated as per review comment. Carrying r+.
Attachment #8363213 - Attachment is obsolete: true
Attachment #8363249 - Flags: review+
This landed on central:

but there was a typo in the bug number in the commit message.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.