Bug 94903 - [chromium] Fix a bug where CCThreadProxy::canDraw() gets stuck at false on tab switch
Summary: [chromium] Fix a bug where CCThreadProxy::canDraw() gets stuck at false on ta...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-23 23:15 PDT by Christopher Cameron
Modified: 2012-08-26 19:49 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.01 KB, patch)
2012-08-24 00:01 PDT, Christopher Cameron
no flags Details | Formatted Diff | Diff
Patch (3.48 KB, patch)
2012-08-24 12:01 PDT, Christopher Cameron
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Cameron 2012-08-23 23:15:08 PDT
[chromium] Fix a bug where CCThreadProxy::canDraw() gets stuck at false on tab switch
Comment 1 Christopher Cameron 2012-08-24 00:01:17 PDT
Created attachment 160346 [details]
Patch
Comment 2 Christopher Cameron 2012-08-24 00:07:02 PDT
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.
Comment 3 Dana Jansens 2012-08-24 08:01:39 PDT
(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.
Comment 4 Antoine Labour 2012-08-24 09:16:21 PDT
(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.
Comment 5 Dana Jansens 2012-08-24 09:24:13 PDT
(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.
Comment 6 Christopher Cameron 2012-08-24 09:53:54 PDT
(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?
Comment 7 Dana Jansens 2012-08-24 10:01:47 PDT
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?
Comment 8 Christopher Cameron 2012-08-24 10:09:21 PDT
(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.
Comment 9 Antoine Labour 2012-08-24 10:23:10 PDT
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?
Comment 10 Christopher Cameron 2012-08-24 12:01:11 PDT
Created attachment 160469 [details]
Patch
Comment 11 Christopher Cameron 2012-08-24 12:03:29 PDT
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.
Comment 12 Antoine Labour 2012-08-24 12:25:55 PDT
Cool, I like.
Comment 13 Dana Jansens 2012-08-24 12:30:08 PDT
Ditto!
Comment 14 James Robinson 2012-08-24 12:32:06 PDT
Comment on attachment 160469 [details]
Patch

R=me, great catch!
Comment 15 WebKit Review Bot 2012-08-26 16:09:37 PDT
Comment on attachment 160469 [details]
Patch

Rejecting attachment 160469 [details] from commit-queue.

ccameron@chromium.org 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.
Comment 16 WebKit Review Bot 2012-08-26 19:49:14 PDT
Comment on attachment 160469 [details]
Patch

Clearing flags on attachment: 160469

Committed r126719: <http://trac.webkit.org/changeset/126719>
Comment 17 WebKit Review Bot 2012-08-26 19:49:17 PDT
All reviewed patches have been landed.  Closing bug.