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

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.

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

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.

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.

The previous patch was not checking CanExposeDrawTarget()

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

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

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

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!

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