Be more disciplined about if and when BorrowDrawTarget() return null

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jrmuizel, Assigned: nical)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
I.e. under what circumstances does this happen, what are callers supposed to do, and in what state is the browser.

Updated

3 years ago
Whiteboard: gfx-noted
(Reporter)

Updated

3 years ago
Assignee: nobody → nical.bugzilla
For example, seems the VMWare+WARP can return null (see bug 1175903.)

What are we looking for here though?  BorrowDrawTarget is a platform specific virtual override.

We can make it never fail, which means that the caller should make a call beforehand saying "will this fail" (as we have a proof by example that it can fail.)  This ends up being the same as just failing, so not much point there.

We can make it so that the callers can always expect it to fail, and have to deal with the failure.  The callers can decide if they should deal with it, or MOZ_CRASH or some such, but it is possible that some information from BorrowDrawTarget could be required to make that decision, at least in some cases.

We can have BorrowDrawTarget MOZ_CRASH when something really bad happened, otherwise the caller has to deal with null.

The full possible extent of comment 0 non-withstanding, sounds like the documentation is the first thing here.
Blocks: 1175903
:nical, can you clarify what should happen, and we can then get somebody to set up the code to do that.

Do we want to leave things so that BorrowDrawTarget can sometimes fail, and the callers should just deal with it (we already do in some places) or do something else?
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 3

3 years ago
My preference would be to have all TextureClients call BorrowDrawTarget() in Lock if the OpenMode contains the WRITE bit, cache the result and fail to lock if BorrowDrawTarget returned false there. This way, if BorrowDrawTarget fails we'll catch that in Lock() which should always be checked, and don't have to null-check subsequent calls to BorrowDrawTarget since it will return the cached-and-already-checked DrawTarget that we got in the Lock call.
Some TextureClient implementations already do that, we just have to make sure the others do as well.
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 4

3 years ago
Created attachment 8701013 [details] [diff] [review]
Check that BorrowDrawTarget succeeds when locking a TextureClient for writing

I forgot to add this back when I moved TextureClient's logic in TextureData (Some TextureClient classes were already doing this). The idea is that if you lock a TextureClient for writing on the main thread, it means you are going to write into it using a DrawTarget. If you are off the main thread it could be a video frame that you'll copy through UpdateFromSurface without a DrawTarget.
It's best to have all of the error handling around calls to Lock than have it for both Lock and BorrowDrawTarget.
Attachment #8701013 - Flags: review?(bas)
(Assignee)

Comment 5

3 years ago
Created attachment 8701424 [details] [diff] [review]
Check that BorrowDrawTarget succeeds when locking a TextureClient for writing

The previous patch was not checking CanExposeDrawTarget()
Attachment #8701013 - Attachment is obsolete: true
Attachment #8701013 - Flags: review?(bas)
Attachment #8701424 - Flags: review?(bas)
Attachment #8701424 - Flags: review?(bas) → review+
backed out for test failurs like https://treeherder.mozilla.org/logviewer.html#?job_id=19037063&repo=mozilla-inbound
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 9

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #7)
> backed out for test failurs like
> https://treeherder.mozilla.org/logviewer.html#?job_id=19037063&repo=mozilla-
> inbound

Yep, sorry. A very similar patch was green on try a while ago so I got a bit too optimistic about this one. I fixed the issue and try is green so far. I'll reland the updated patch soon.
Flags: needinfo?(nical.bugzilla)
sorry nical but somehow this cause the same issue as yesterday https://treeherder.mozilla.org/logviewer.html#?job_id=19073947&repo=mozilla-inbound could you take a look, thanks!
Flags: needinfo?(nical.bugzilla)

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fcd8d3854108
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Updated

3 years ago
Flags: needinfo?(nical.bugzilla)
You need to log in before you can comment on or make changes to this bug.