1072491 - Make GrallocTextureSource/Source more robust against missing gl context and gl failures

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

There are a few places in the gralloc code where we assume things to "just work", which is true most of the time but probably bites us in rare cases and may bite usin the future.
Let's explicitly check that we have a valid reference to the gl context before using it (as it is hard to make sure that it doesn't go away under our feet since it is controlled by the compositor), and check the return value of MakeCurrent which will rarely fail on b2g (which could change) and does fail after a certain stage of the shutdown sequence.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Add some more checks in gralloc code

most of the patch consists of additional checks which can't hurt, but there are a few things to be careful about for the review:
* I made the Lock method on the TextureSource return a bool, which, if returning false, will make the TextureHost's Lock return false, effectively cancelling the compositing of the texture. So if there are early return cases in the TextureSource that are not error cases they should return true and not false. I think this is the case for the if (mCompositableBackendData) branch but you should double check this part.
* I made it so that SetCompositor will delete the current texture if we are changing to another compositor, which I think makes sense, because having a texture coming from a compositor and not having access to that compositor after a while means that you won't be able to delete it after you change the compositor, but I am not 100% sure that there is no tricky code relying on the current behavior here.

I think that this patch will help with bugs such as bug 1019236 and the related crashes.

Comment 2

[Nicolas Silva [:nical]]

try https://tbpl.mozilla.org/?tree=Try&rev=5a73d2b0c27b

Comment 3

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #1)
> Created attachment 8494719 [details] [diff] [review]
> Add some more checks in gralloc code
> 
> most of the patch consists of additional checks which can't hurt, but there
> are a few things to be careful about for the review:
> * I made the Lock method on the TextureSource return a bool, which, if
> returning false, will make the TextureHost's Lock return false, effectively
> cancelling the compositing of the texture. So if there are early return
> cases in the TextureSource that are not error cases they should return true
> and not false. I think this is the case for the if
> (mCompositableBackendData) branch but you should double check this part.

Just doing mCompositableBackendData might not always ensure the lock success. There might be a possibility that ForgetBuffer() is already called. We might not be necessary to care about it.

Comment 4

[Nicolas Silva [:nical]]

Fixed some errors, got a green try push: https://tbpl.mozilla.org/?tree=Try&rev=3caaafd71d90

rebased and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea6345cc308

For now GrallocTextureHost::Lock returns true even when GrallocTextureSource returns false (as it did before the patch). Not ideal but it caused some failures on the previous push so I'll investigate that later.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Definitely hoping we can get this on Aurora34 as well.

Comment 6

[Nicolas Silva [:nical]]

Attachment: Upliftable patch (for aurora, carrying r=sotaro)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ea6345cc308

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Can you please nominate this for Aurora uplift? :)

Comment 9

[Nicolas Silva [:nical]]

Comment on attachment 8495989 [details] [diff] [review]
Upliftable patch (for aurora, carrying r=sotaro)

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: some crashes on b2g
[Describe test coverage new/current, TBPL]:
[Risks and why]: low, mostly nullptr checks
[String/UUID change made/needed]:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dfaa51f08e7a

Comment 11

[Duane Dixon [:ddixon]]

Unable to verify this issue.  

This appears to be a backend bug that deals with code.