Bug 94903 – [chromium] Fix a bug where CCThreadProxy::canDraw() gets stuck at false on tab switch

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Christopher Cameron]

[chromium] Fix a bug where CCThreadProxy::canDraw() gets stuck at false on tab switch

[Christopher Cameron]

Created attachment 160346 [details]
Patch

[Christopher Cameron]

Belated description, CCing people related to this area.

Suppose the impl thread deletes all textures via releaseContentsTextures().  The impl thread will not be able to draw again until resetContentsTexturesPurged() is called in scheduledActionCommit().  When deleting the textures, the function releaseContentsTextures() calls setNeedsCommitOnImplThread() to ensure that a commit will come along to allow drawing again.  If this commit is aborted, then the page will not draw until a commit is scheduled, which may be forever.

The fix in the patch makes beginFrameAbortedOnImplThread() trigger a commit if there is an outstanding purge, so that drawing will be re-enabled in scheduledActionCommit().

The patch also re-arranges some things in preparation for a change I'm working on to allow purging only some of the textures at a time -- at first I thought that this issue (which manifests in a newly-switched-to-tab's contents just not appearing) was a bug in that patch.  LMK if you'd prefer a smaller diff (as, for instance, the ability to ACK a deletion in abort may not make sense in the smaller context).

I tested the patch with CCLayerTreeHostTestEvictTextures.runMultiThread.

[Dana Jansens]

(In reply to comment #2)
> Suppose the impl thread deletes all textures via releaseContentsTextures().  The impl thread will not be able to draw again until resetContentsTexturesPurged() is called in scheduledActionCommit().  When deleting the textures, the function releaseContentsTextures() calls setNeedsCommitOnImplThread() to ensure that a commit will come along to allow drawing again.  If this commit is aborted, then the page will not draw until a commit is scheduled, which may be forever.

What about setting needs-commit when the compositor becomes visible? beginFrameAbort only happens when not visible.

[Antoine Labour]

(In reply to comment #3)
> (In reply to comment #2)
> > Suppose the impl thread deletes all textures via releaseContentsTextures().  The impl thread will not be able to draw again until resetContentsTexturesPurged() is called in scheduledActionCommit().  When deleting the textures, the function releaseContentsTextures() calls setNeedsCommitOnImplThread() to ensure that a commit will come along to allow drawing again.  If this commit is aborted, then the page will not draw until a commit is scheduled, which may be forever.
> 
> What about setting needs-commit when the compositor becomes visible? beginFrameAbort only happens when not visible.

Right, and that should already happen (or does it not under certain circumstances? That's probably what we want to fix).
If you force a commit when we're invisible, then you'll reallocate all the textures, defeating the purpose.

[Dana Jansens]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Suppose the impl thread deletes all textures via releaseContentsTextures().  The impl thread will not be able to draw again until resetContentsTexturesPurged() is called in scheduledActionCommit().  When deleting the textures, the function releaseContentsTextures() calls setNeedsCommitOnImplThread() to ensure that a commit will come along to allow drawing again.  If this commit is aborted, then the page will not draw until a commit is scheduled, which may be forever.
> > 
> > What about setting needs-commit when the compositor becomes visible? beginFrameAbort only happens when not visible.
> 
> Right, and that should already happen (or does it not under certain circumstances? That's probably what we want to fix).
> If you force a commit when we're invisible, then you'll reallocate all the textures, defeating the purpose.

The scheduler doesn't do commits while non-visible so that shouldn't be a problem. I think we schedule a commit when we go non-visible, but I'm having trouble finding the place where we do.

[Christopher Cameron]

(In reply to comment #5)
> (In reply to comment #4)
> > If you force a commit when we're invisible, then you'll reallocate all the textures, defeating the purpose.
> 
> The scheduler doesn't do commits while non-visible so that shouldn't be a problem. I think we schedule a commit when we go non-visible, but I'm having trouble finding the place where we do.

From my manual testing it does appear that a commit requested by the impl thread while not visible did not actually occur until the tab became visible.

Would it be preferable for setNeedsCommit to mean "a commit needs to complete", rather than "a commit needs to start", so that commits that are aborted because the tab went invisible are automatically rescheduled when next the tab becomes visible (if it ever does)?  Are there circumstances where that is undesirable?

[Dana Jansens]

I do agree conceptually that sounds like a reasonable behaviour. I am wondering if you have a test case that demonstrates the bug this is trying to fix though? For single thread or threaded mode?

But, if so, an easy way to make sure a commit happens when aborted might be to just have CCSchedulerStateMachine::beginFrameAborted set m_needsCommit = true?

[Christopher Cameron]

(In reply to comment #7)
> I do agree conceptually that sounds like a reasonable behaviour. I am wondering if you have a test case that demonstrates the bug this is trying to fix though? For single thread or threaded mode?

I have a manual test case -- open 2 expensive tabs (lots of 3D CSS, etc) and hold ctrl+pagedown.  If an abort happens on just one tab, and when you release ctrl+pagedown you are at that tab, then you will see the other tab's contents (and canDraw() will be spinning at false).

I didn't try this without threaded compositing.

> But, if so, an easy way to make sure a commit happens when aborted might be to just have CCSchedulerStateMachine::beginFrameAborted set m_needsCommit = true?

I'll go with that instead, unless we can think of any reasons that it is undesirable.

[Antoine Labour]

As long as we don't keep posting tasks to beginFrame that get Aborted, it sounds like a reasonable suggestion.

Can we add a test to check that behavior?

[Christopher Cameron]

Created attachment 160469 [details]
Patch

[Christopher Cameron]

Chagned th(In reply to comment #9)
> As long as we don't keep posting tasks to beginFrame that get Aborted, it sounds like a reasonable suggestion.
> 
> Can we add a test to check that behavior?

We actually already have a test for this behavior -- TestGoesInvisibleBeforeBeginFrameCompletes.  I've updated this to set visible again and verify that when we do this we are asked to start a frame and then begin a commit.

[Antoine Labour]

Cool, I like.

[Dana Jansens]

Ditto!

[James Robinson]

Comment on attachment 160469 [details]
Patch

R=me, great catch!

[WebKit Review Bot]

Comment on attachment 160469 [details]
Patch

Rejecting attachment 160469 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 160469 [details]
Patch

Clearing flags on attachment: 160469

Committed r126719: <http://trac.webkit.org/changeset/126719>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.