[chromium] Missing wait() in CCThreadProxy::setVisible

• Component: New Bugs
• Status: RESOLVED FIXED

[Iain Merrick]

[chromium] Missing wait() in CCThreadProxy::setVisible

[Iain Merrick]

Created attachment 114262 [details]
Patch

[Iain Merrick]

setVisible() returns immediately, so the CCCompletionEvent is deleted before the compositor thread has had a chance to call signal().

I've added an ASSERT which should catch similar bugs in future.

Haven't yet figured out a good way to test this. Neither our unit tests nor any existing layout tests seem to call setVisible(). I guess I'll have to create a new CCThreadProxyTest.

[Nat Duca]

Comment on attachment 114262 [details]
Patch

s/signalled/signaled

The fix makes sense.

I like the guards.

Unofficial lgtm.

[James Robinson]

Comment on attachment 114262 [details]
Patch

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

No chance of a unit test? We have some CCLayerTreeHostTests that bounce between threads already.

> Source/WebCore/platform/graphics/chromium/cc/CCCompletionEvent.h:56
> +        ASSERT(!m_waited && (m_waited = true));

side-effects in an ASSERT() is pretty ugly - can you just do the assignment in an #ifndef NDEBUG block?  this reads like a bug, not something intentional

> Source/WebCore/platform/graphics/chromium/cc/CCCompletionEvent.h:63
> +        ASSERT(!m_signalled && (m_signalled = true));

same here

[Iain Merrick]

Comment on attachment 114262 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCCompletionEvent.h:56
>> +        ASSERT(!m_waited && (m_waited = true));
> 
> side-effects in an ASSERT() is pretty ugly - can you just do the assignment in an #ifndef NDEBUG block?  this reads like a bug, not something intentional

The #ifdef felt a bit noisy, but you're right, this is just icky. Will fix.

[Iain Merrick]

Created attachment 114502 [details]
Made asserts less hacky

[Iain Merrick]

Comment on attachment 114502 [details]
Made asserts less hacky

I'm going to add a new test case in CCLayerTreeHostTest, but it's not quite working as expected -- there may be further sneaky problems with setVisible().

[Iain Merrick]

(In reply to comment #7)
> (From update of attachment 114502 [details])
> I'm going to add a new test case in CCLayerTreeHostTest, but it's not quite working as expected -- there may be further sneaky problems with setVisible().

Aha, I see what's going on: setVisible(true) is asynchronous, so it only gets pushed to the CCScheduler at commit time, but commits are suppressed while we're invisible.

Allowing commits while !visible works around this, but I'm not sure it's correct. I wonder if the core state machine should be checking visibility at all? Doing draws and commits while invisible isn't *wrong*, it's just inefficient. So maybe we should suppress those at a higher level, in the main thread.

Anyway, I'll amend the patch and you can judge for yourself.

[Iain Merrick]

Created attachment 114522 [details]
Added unit test, fixed setVisible(true)

[Iain Merrick]

(In reply to comment #9)
> Created an attachment (id=114522) [details]
> Added unit test, fixed setVisible(true)

Also Americanizzzed the spelling of "signaled" to keep Nat happy.

[James Robinson]

Comment on attachment 114522 [details]
Added unit test, fixed setVisible(true)

Suppressing commits when not visible is an efficiency that we can't afford to not have.

Should we pass visible state across explicitly instead of relying on setNeedsCommit() / pushing in the commit?

[Nat Duca]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 114502 [details] [details])
> > I'm going to add a new test case in CCLayerTreeHostTest, but it's not quite working as expected -- there may be further sneaky problems with setVisible().
> 
> Aha, I see what's going on: setVisible(true) is asynchronous, so it only gets pushed to the CCScheduler at commit time, but commits are suppressed while we're invisible.
> 
> Allowing commits while !visible works around this, but I'm not sure it's correct. I wonder if the core state machine should be checking visibility at all? Doing draws and commits while invisible isn't *wrong*, it's just inefficient. So maybe we should suppress those at a higher level, in the main thread.
> 
> Anyway, I'll amend the patch and you can judge for yourself.

Wait, what's the expectation when setVisible(true)? Afaik, that should be a noop...

[James Robinson]

Created attachment 114574 [details]
Patch

[James Robinson]

Created attachment 114579 [details]
Patch

[James Robinson]

Comment on attachment 114579 [details]
Patch

This makes setVisible always synchronous and pushes visible state through that message only instead of hooking into the commit flow.

[Nat Duca]

Comment on attachment 114579 [details]
Patch

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

Coolies. Thanks James and Iain.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:263
> +        m_schedulerOnImplThread->setNeedsCommit();

why needs commit? if you just go away and come back and the page is truly idle, that wont be necessary.

[James Robinson]

Created attachment 114745 [details]
Patch

[James Robinson]

(In reply to comment #16)
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:263
> > +        m_schedulerOnImplThread->setNeedsCommit();
> 
> why needs commit? if you just go away and come back and the page is truly idle, that wont be necessary.

Removed - it's not necessary

[Nat Duca]

Comment on attachment 114745 [details]
Patch

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

LGTM

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:214
> +        callOnMainThread(visible ? CCLayerTreeHostTest::dispatchSetVisible : CCLayerTreeHostTest::dispatchSetInvisible, this);

is this because callOnMainThread doesn't support args? Its fine if that's the case.

[James Robinson]

Comment on attachment 114745 [details]
Patch

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

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:214
>> +        callOnMainThread(visible ? CCLayerTreeHostTest::dispatchSetVisible : CCLayerTreeHostTest::dispatchSetInvisible, this);
> 
> is this because callOnMainThread doesn't support args? Its fine if that's the case.

Yeah I think (this part is from Iain's patch, but that's the reason I would have guessed).

[WebKit Review Bot]

Comment on attachment 114745 [details]
Patch

Clearing flags on attachment: 114745

Committed r100055: <http://trac.webkit.org/changeset/100055>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.