Closed Bug 1072491 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(2 files)

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.
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.
Assignee: nobody → nical.bugzilla
Attachment #8494719 - Flags: review?(sotaro.ikeda.g)
(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.
Attachment #8494719 - Flags: review?(sotaro.ikeda.g) → review+
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.
Definitely hoping we can get this on Aurora34 as well.
https://hg.mozilla.org/mozilla-central/rev/8ea6345cc308
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Can you please nominate this for Aurora uplift? :)
Flags: needinfo?(nical.bugzilla)
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]:
Attachment #8495989 - Flags: approval-mozilla-aurora?
Flags: needinfo?(nical.bugzilla)
Attachment #8495989 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Unable to verify this issue.  

This appears to be a backend bug that deals with code.
QA Whiteboard: [QAnalyst-Triage?] [QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] [QAnalyst-verify-] → [QAnalyst-Triage+] [QAnalyst-verify-]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.