1611127 - Optimized surfaces may be invalidated/released, but the frame remains in surface cache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P2)

Description

[Andrew Osmond [:aosmond] (he/him)]

When we attempt to try an optimized surface, it may call SourceSurface::IsValid, and if that returns false, it will clear the mOptSurface pointer:

https://searchfox.org/mozilla-central/rev/e878e5b81bb319c141900ce9cfcde732df5c8449/image/imgFrame.cpp#839

This will be called on the draw path.

We don't actually check if mOptSurface is null, we just assume it can't be, when we create the DrawableFrameRef:

https://searchfox.org/mozilla-central/rev/e878e5b81bb319c141900ce9cfcde732df5c8449/image/imgFrame.h#359

We assert that mOptSurface isn't null, but If we don't check, then it will default to assuming there is a valid underlying surface, so non-debug builds can get stuck.

SourceSurface::IsValid appears to always return true except for SourceSurfaceCapture (not used on this code path) and SourceSurfaceD2D1 (which can be created via imgFrame::Optimize on Windows if the image is used with a DrawTargetD2D). The latter's condition could fail if there was a device reset, GPU process crash, etc, thus suggesting this could have real world implications.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

The user symptom should be that the CachedImage remains in the SurfaceCache, but it has no underlying SourceSurface. This means we will not attempt to redecode, we will fail to draw anything and the image appears to be missing from the user perspective.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1611127 - Fix how images with optimized surfaces may go missing on Windows.

On Windows, optimized surfaces can become invalid due to a device reset
or GPU process crash when D2D draw targets were being used. This is
because SourceSurfaceD2D1 checks if the D2D context has changed from the
one it was optimized for.

We assumed when creating a DrawableFrameRef that if we don't have
imgFrame::mRawSurface, then we must have an imgFrame::mOptSurface. This
is incorrect and causes SurfaceCache to believe it has a valid
DrawableSurface, even if the underlying optimized surface cannot be used
for drawing (or has already been freed).

If we can't draw the image, it goes missing from the screen. But since
the cache thinks it has a valid surface, we never actually redecode it.
With this patch, we now check if the imgFrame::mOptSurface has been
freed already, and if not, that it remains valid for drawing purposes.
If either condition is true, the SurfaceCache entry will be removed,
thus permitting a redecode attempt.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

I think the image might have to be layerized to hit this problem or otherwise use the GetFrame / GetImageContainer paths. The Draw path seems like it might flush the image cache for the image as a bit of a hack. At the very least, this change will prevent dead entries from showing up in the memory reports :).

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

For Windows, mconca was able to confirm that forcing a device reset makes his images disappear. This lines up with the about:support graphics error log when the issue was encountered naturally. This gives me hope it is the primary cause on Windows, especially in release.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Upon further consideration, SourceSurfaceCapture::IsValid actually has its own conditions. I don't know the code well enough to know if there is some edge case where it fails (it looks like it generally should not), but I think I can make the claim that anything that uses OMTP could see an impact now. A long shot though.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

OMTP is potentially used on all desktop platforms.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90bfaa4e6045
Fix how images with optimized surfaces may go missing on Windows. r=tnikkel

Comment 8

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/90bfaa4e6045

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9122639 [details]
Bug 1611127 - Fix how images with optimized surfaces may go missing on Windows.

Beta/Release Uplift Approval Request

• User impact if declined: Images may go missing if the underlying surface becomes invalid (e.g. device reset on Windows).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is very small. The logic flow change is a very well exercised / tested code path.
• String changes made/needed:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9122639 [details]
Bug 1611127 - Fix how images with optimized surfaces may go missing on Windows.

Small fix which may help avoid images going missing in some cases. Approved for 73.0RC1.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/cc70a44a5ac8