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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

: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?

Comment 3

[Nicolas Silva [:nical]]

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.

Comment 4

[Nicolas Silva [:nical]]

Attachment: 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.

Comment 5

[Nicolas Silva [:nical]]

Attachment: Check that BorrowDrawTarget succeeds when locking a TextureClient for writing

The previous patch was not checking CanExposeDrawTarget()

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d569d015069d

Comment 7

[Carsten Book [:Tomcat]]

backed out for test failurs like https://treeherder.mozilla.org/logviewer.html#?job_id=19037063&repo=mozilla-inbound

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/231e202a15bf

Comment 9

[Nicolas Silva [:nical]]

(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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f17f489417

Comment 11

[Carsten Book [:Tomcat]]

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!

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/668ba6273e07

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd8d3854108

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fcd8d3854108