Bug 93684 – [chromium] race between CCLayerTreeHostImpl::releaseContentsTextures and CCThreadProxy::beginFrameCompleteOnImplThread

• Component: Platform
• Status: RESOLVED FIXED

[Antoine Labour]

If releaseContentsTextures gets called between CCThreadProxy::scheduledActionBeginFrame and CCThreadProxy::beginFrameCompleteOnImplThread, we evict all the resources, and set the needsCommit flag, but the main thread won't know about it and paint the tiles as if the resources were not deleted, and queues a bunch of updates. When beginFrameCompleteOnImplThread gets called, we execute those updates on deleted resources, causing ASSERTs in debug and very bad things in release. Also the needsCommit flag is essentially lost in the dust, meaning we won't repaint those missing textures.

Quick solution: just delay the eviction until after the commit. However we may not want to delay the eviction in which case the proposed fix is:
- if the m_contentsTexturesWerePurgedSinceLastCommit flag is set in beginFrameCompleteOnImplThread, drop the updates that touch any managed texture
- keep track of whether we set m_contentsTexturesWerePurgedSinceLastCommit since scheduledActionBeginFrame, in which case we don't want to reset m_contentsTexturesWerePurgedSinceLastCommit in commitComplete, and post another setNeedsCommit then.

The way I reproed this is by running chrome/aura with --force-compositing-mode --enable-threaded-compositing --enable-ui-release-front-surface, open 3 tabs and switch rapidly between them.

[Antoine Labour]

Created attachment 157850 [details]
Patch

[Antoine Labour]

I'll write tests before landing, but please check the approach and/or if I'm missing something.
My assumption if the frame is "aborted" (because we got invisible in the mean time) is that I can keep the state as is, and a commit will be kicked when the tab becomes visible again, causing a new beginFrame post that will validate the textures then.

[Adrienne Walker]

Comment on attachment 157850 [details]
Patch

Can this get unit tested? This might fit with some of the CCLayerTreeHostTest tests that test other proxy interactions.

[Antoine Labour]

Yes, that's the plan (as I wrote in #2).

[James Robinson]

Comment on attachment 157850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157850&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:272
> +    bool m_beginFramePosted;
> +    bool m_contentsTexturesWerePurgedSinceLastBeginFrame;
> +    bool m_contentsTexturesPurged;

could you keep the more stateful bits up on CCThreadProxy?  It's the class responsible for handling state transitions (which it mostly delegates to the scheduler) and the details of the commit flow, so I think it should keep track of what it has posted to the main thread and what it needs to preserve.  Also, the single thread proxy doesn't have to worry about any of this stuff.

[Antoine Labour]

Created attachment 158195 [details]
Patch

[Antoine Labour]

(In reply to comment #3)
> (From update of attachment 157850 [details])
> Can this get unit tested?

Done

(In reply to comment #5)
> (From update of attachment 157850 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157850&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:272
> > +    bool m_beginFramePosted;
> > +    bool m_contentsTexturesWerePurgedSinceLastBeginFrame;
> > +    bool m_contentsTexturesPurged;
> 
> could you keep the more stateful bits up on CCThreadProxy?  It's the class responsible for handling state transitions (which it mostly delegates to the scheduler) and the details of the commit flow, so I think it should keep track of what it has posted to the main thread and what it needs to preserve.  Also, the single thread proxy doesn't have to worry about any of this stuff.

Done.

[James Robinson]

Comment on attachment 158195 [details]
Patch

R=me - nice test!

[WebKit Review Bot]

Comment on attachment 158195 [details]
Patch

Clearing flags on attachment: 158195

Committed r125577: <http://trac.webkit.org/changeset/125577>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.