Bug 77049 – [Chromium] Force compositeAndReadback through regular scheduling flow

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

Scheduler currently assumes that beginFrameComplete is only called after scheduledActionBeginFrame(). That's not the case for compositeAndReadback which might trigger a call to beginFrameComplete in different situations. This can currently cause an assertion failure in debug builds.

[David Reveman]

Created attachment 124011 [details]
Patch

[David Reveman]

The attached patch addresses this by simply changing the assumptions made about when beginFrameComplete can be called. Maybe we want to follow up with some larger changes to how compositeAndReadback interacts with the scheduler but I think this change is good enough as a bugfix+unittest patch.

[David Reveman]

Created attachment 124157 [details]
Patch

[David Reveman]

*** Bug 72908 has been marked as a duplicate of this bug. ***

[David Reveman]

Included a fix for https://bugs.webkit.org/show_bug.cgi?id=72908 as it's very much part of the same problem.

[Nat Duca]

Can you describe the sequence of events that is currently broken?

[David Reveman]

(In reply to comment #6)
> Can you describe the sequence of events that is currently broken?

The unit test covers the sequences I found to be broken. Here's one of them:

ImplThread - setVisible(false), e.g. the page is a background tab.
MainThread - setNeedsCommit, some change to the page cause this to be called.
MainThread - compositeAndReadback, is called before the scheduler schedules a BeginFrame action.
MainThread - beginFrameAndCommit, is called as pending commit exists.
ImplThread - beginFrameCompleteOnImplThread, blows up in debug mode as scheduler commit state is not COMMIT_STATE_FRAME_IN_PROGRESS when beginFrameComplete() is called.

[James Robinson]

Can we make compositeAndReadback() forcibly kick the scheduler into the right state instead of making the scheduler ASSERT()s universally more liberal? compositeAndReadback() is very much a special case, and one that we'd like to get rid of.  I can't speak for Nat but I know that I'd be happier with that particular codepath setting funky bits explicitly instead of all scheduler / state machine logic needing to be aware of otherwise nonsensical states.

[Nat Duca]

Thanks for the reproduce case!

I'm going to try to fix this by changing the compositeAndReadback flow to never create a beginFrameAndCommitTask in the first place.

[Nat Duca]

Created attachment 131007 [details]
Patch

[James Robinson]

Comment on attachment 131007 [details]
Patch

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

Very nice! I think we have a wee bit o' a memory leak, but easily fixable.

R=me

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:397
> +    m_pendingBeginFrameRequest = new BeginFrameAndCommitState();

if we schedule a begin frame from the impl thread, but then start a shutdown from the main thread before we process the message will we end up leaking this pointer?  maybe m_pendingBeginFrameRequest should be an OwnPtr<> member - i think if we have to shut down the compositor with a beginFrame pending the right thing to do is to free up its memory and just don't worry about the scroll offsets/etc on it

[Nat Duca]

(In reply to comment #11)
> (From update of attachment 131007 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131007&action=review
> 
> Very nice! I think we have a wee bit o' a memory leak, but easily fixable.
Hahaha whomp. Will fix that up. Thanks!

[Nat Duca]

Created attachment 131381 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 131381 [details]
Patch for landing

Clearing flags on attachment: 131381

Committed r110464: <http://trac.webkit.org/changeset/110464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.