1061694 - Don't let GLBlitTextureImageHelper::BlitTextureImage be called with null source TextureImage

Status: NEW

(Core :: Graphics: Layers, defect)

Description

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

See bug 1033098.  We were intermittently getting GLBlitTextureImageHelper::BlitTextureImage called with aSrc a nullptr and crashing. Probably need to stop this back in ContentClient::BeingPaintBuffer or clarify if aSrc == nullptr is a valid argument.

Comment 1

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

See https://tbpl.mozilla.org/?tree=Try&rev=c15c7effa457 and search for "LOGGING" in the full log of the failed Gip tests.

Comment 2

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

From what I can tell, looking at the code and the call stack when the failure happens:

* A public method GLBlitTextureImageHelper::BlitTextureImage sometimes gets called with aSrc == nullptr.  Until recently, we just made the assumption that is never the case (e.g., we assumed aSrc is valid.)  There is no documentation of that method stating one way or another what the validity of aSrc (or aDst, for that matter) is assumed to be.

* A public method TextureImageTextureHostOGL::CopyTo has destination as the argument (a pointer, without mention as to whether it should be checked by the caller or within the method itself), which we assume to be valid.  We also assume that this->mTexImage is valid (without checking this->IsValid()), and this is not the case when the intermittent failure happens.  TextureImageTextureHostOGL->mTexImage should be valid if ::EnsureBuffer or ::Update method has been called, since creation or since DeallocateDeviceData().  Not sure if it's a problem that it isn't valid at this point, or that we haven't checked for it.

* ContentHostIncremental::TextureCreationRequest::Execute is next, with aHost parameter.  We check if mTextureInfo.mDeprecatedTextureHostFlags & DeprecatedTextureHostFlags::COPY_PREVIOUS is true, and at that point assume that aHost->mSource->IsValid(), don't check, and call aHost->mSource->CopyTo (basically, with aHost->mSource->mTexImage not being valid.)  We do not call aHost->mSource->EnsureBuffer(), though we do call it for the argument that eventually ends up being the destination for the CopyTo/BlitTextureImage.  So, at least in this stack (don't know if there are other ways to call this), BlitTextureImage should get a valid destination - not the case for the source though, which is the issue here.

At this point - there doesn't seem to be documentation as to whether mTextureInfo.mDeprecatedTextureHostFlags & DeprecatedTextureHostFlags::COPY_PREVIOUS should guarantee that aHost->mSource->IsValid() or if we should enter that branch only if both are true, or if we should enter the branch based on the set bit, but then call aHost->mSource->EnsureBuffer (?) to make sure aHost->mSource is in fact valid.