Bug 66145 – [chromium] Assert that main thread and compositor thread are used safely

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

[Iain Merrick]

Our threaded compositor has three modes of operation.

Paint: rendering content using standard WebKit calls. This happens on the main thread.
Commit: pushing data over to the compositor tree. The two threads are synchronized.
Update: presenting content on the screen using GL calls. This happens on the compositor thread.

Each mode involves a fairly complex traversal of the layer tree and/or compositor tree. There may be hidden bugs because we don't actually have a compositor thread yet. We should add asserts to check that methods are called in the appropriate mode(s).

[Iain Merrick]

Created attachment 103780 [details]
CCThreadRestriction class that checks only specified mode(s) used during its lifetime.

Suggested implementation: each thread has a set of permitted modes. We can temporarily restrict the modes a thread can use (e.g., lock it down to "Paint" only). If we find ourselves trying to widen the set of modes (e.g. we're currently in "Update" and we try to restrict to "Paint") we assert.

Pro: easy to use correctly, non-invasive.
Con: failures happen inside CCThreadRestriction. Hard to see where the errors are really occurring without debugging.

I also added an ASSERT macro but that can only do an one-off check. Not obvious how to combine that with a scoped object.

I started sprinkling checks through LayerRendererChromium, but rapidly hit asserts.

[James Robinson]

I don't think function-level blocks will work, since we will sometimes changes modes in the middle of a function.  I'd suggest having a set of assertions (something like assertMainThread(), assertDuringCommit(), assertCompositorThread() - I'm sure you can think of better names) and explicit calls to switch state.

[Iain Merrick]

If we switch modes in a function we could just block-scope it, no?

void foo() {
  {
    CCThreadRestriction mode(Paint);
    paint();
  }
  {
    CCThreadRestriction mode(Commit);
    commit();
  }
}

[Nat Duca]

In previous incarnations of this code, I proposed something ~= Chrome's NonThreadSafe<T>. This got shot down... the general idea is that a function should *never* have access to stuff that isn't safe in the first place.

So, my question would be, where in our code do we have a function that can get to pointers at the wrong time?

[James Robinson]

I guess that's a little stronger than what I was thinking - all I had in mind was DEBUG-only assertions that we're in the right place.  Nothing for release builds.

[Iain Merrick]

OK, lemme simplify this a bit to:

ASSERT_IN_MAIN_THREAD()
ASSERT_IN_COMPOSITOR_THREAD()
ASSERT_IN_COMMIT_PHASE()

And block-scoped objects that control when each assert is valid. Right now there's only one thread, so 'main thread' and 'compositor thread' need to be faked, right?

[James Robinson]

Yeah, that looks like a great start.  We can expand to get more sophisticated as we need.

You can also use the USE(THREADED_COMPOSITING) macro to tell if there's supposed to be a real thread - when that is not set, then you will have to 'fake' the threaded stuff (just check a bool).  When it is set, you can use our existing isOnMainThread() assertions.

[Iain Merrick]

Created attachment 103820 [details]
isCCThread() and ScopedFakeCompositorThread

I noticed that CCLayerTreeHostImplProxy has its own isMainThread() and isCCThread(). With this patch those are both global functions.

[Iain Merrick]

Created attachment 103838 [details]
Cleanup. Don't declare ScopedFakeCompositorThread if THREADED_COMPOSITOR is enabled.

[James Robinson]

Hey Iain,
What's the status here?  You haven't marked the patch "review?" so I haven't tried to review it.  Are you waiting for some feedback or continuing to do work on this or what?

[Iain Merrick]

Oh! Sorry, not familiar with the process. I'll go ahead and mark it for review. (I was busy with cherry-picking elsewhere.)

[WebKit Review Bot]

Attachment 103838 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/graphics/chromium/..." exit_code: 1

Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:46:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 103838 [details]
Cleanup. Don't declare ScopedFakeCompositorThread if THREADED_COMPOSITOR is enabled.

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

Please fix the style issue the bot caught and comments.  I also think you should make sure that this all compiles out in release mode, since the only real side effect is ASSERT()s which are debug only.  You might have to be creative to avoid adding too many #ifndef NDEBUG lines, but it should be possible.  You don't need to guard ASSERT()s since they already compile out.

> WebCore/platform/graphics/chromium/cc/CCThread.cpp:46
> +    ASSERT(ccThreadID == 0);

should be ASSERT(!ccThreadID)

> WebCore/platform/graphics/chromium/cc/CCThread.cpp:92
> +

nit: extra newline

> WebCore/platform/graphics/chromium/cc/CCThread.cpp:97
> +

nit: extra newline

> WebCore/platform/graphics/chromium/cc/CCThread.h:36
> +// True if we're on the compositor thread.
> +bool isCCThread();

I don't think it's a good idea to put this directly in the WebCore namespace.  Can you make this a static function on CCThread, just so that it's namespaced more tightly?

[Nat Duca]

Comment on attachment 103838 [details]
Cleanup. Don't declare ScopedFakeCompositorThread if THREADED_COMPOSITOR is enabled.

How does all of this going to work when the threaded mode is turned off?

Right now, the plan was for the CCLayerTreeHostImplProxy to have two implementations.

Can ou keep the isCCThread/isMainThread decider in the proxy somehow so that we can more easily switch (at runtime) the proxy?

[Iain Merrick]

Created attachment 104202 [details]
Moved isCCThread to CCLayerTreeHostImplProxy::isImplThread

In offline discussion with Nat we decided that the functions should live in CCLayerTreeHostImplProxy. Renamed isCCThread to isImplThread for clarity. I'm still not happy that these are static but maybe that can be fixed once the threaded and non-threaded versions of CCLayerTreeHostImplProxy are fully sorted out.

Per James' request we don't do anything fancy in NDEBUG builds.

[James Robinson]

Comment on attachment 104202 [details]
Moved isCCThread to CCLayerTreeHostImplProxy::isImplThread

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

The logic seems a little broken for the !USE(THREADED_COMPOSITOR) case. I also don't think that having a helper class and whatnot is necessary.  Do the simple thing.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:55
> +    static bool isMainThread();
> +    static bool isImplThread();

#ifndef NDEBUG around these?

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:63
> +#if !USE(THREADED_COMPOSITING) && !defined(NDEBUG)
> +    // Pretend that we're on the impl thread for the lifetime of this object.
> +    // Use this to make ASSERT(isImplThread()) valid when we're not threaded.
> +    // TODO: CCLayerTreeHostImplProxy is going to be split into two classes, one
> +    // threaded and one unthreaded. Move this feature to the unthreaded class.
> +    class ScopedImplThread {
> +       WTF_MAKE_NONCOPYABLE(ScopedImplThread);

Do we really need this?  Why not just a setter?

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:141
> +bool CCLayerTreeHostImplProxy::isMainThread()
>  {
>      return ::isMainThread();
>  }
>  
> -bool CCLayerTreeHostImplProxy::isCCThread() const
> +#if USE(THREADED_COMPOSITING)
> +bool CCLayerTreeHostImplProxy::isImplThread()
>  {
>      return currentThread() == ccThread->threadID();
>  }
> +#elif defined(NDEBUG)
> +bool CCLayerTreeHostImplProxy::isImplThread()
> +{
> +    return true;
> +}

It seems like in !USE(THREADED_COMPOSITOR) isMainThread() and isImplThread() will always return true.  That's not very useful - I think you want to check the fakeImplThread in the !USE(THREADED_COMPOSITOR) case.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:155
> +CCLayerTreeHostImplProxy::ScopedImplThread::ScopedImplThread() : m_previous(fakeImplThread)
> +{
> +    fakeImplThread = true;
> +}

Do we really need to support arbitrary nesting here?  This seems overdesigned.  Just have setter for the bool.

[Iain Merrick]

Comment on attachment 104202 [details]
Moved isCCThread to CCLayerTreeHostImplProxy::isImplThread

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

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:55
>> +    static bool isImplThread();
> 
> #ifndef NDEBUG around these?

Done.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:63
>> +       WTF_MAKE_NONCOPYABLE(ScopedImplThread);
> 
> Do we really need this?  Why not just a setter?

Done.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:141
>> +}
> 
> It seems like in !USE(THREADED_COMPOSITOR) isMainThread() and isImplThread() will always return true.  That's not very useful - I think you want to check the fakeImplThread in the !USE(THREADED_COMPOSITOR) case.

I figured it had to return *something*, and true was safe (the main thread is actually the impl thread).

But if isImplThread isn't even defined for release builds, the problem goes away.

Done.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:155
>> +}
> 
> Do we really need to support arbitrary nesting here?  This seems overdesigned.  Just have setter for the bool.

Done.

[Iain Merrick]

Created attachment 104245 [details]
Addressed James' comments

[James Robinson]

Comment on attachment 104245 [details]
Addressed James' comments

R=me

[WebKit Review Bot]

Comment on attachment 104245 [details]
Addressed James' comments

Rejecting attachment 104245 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
ommit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9425279

[Iain Merrick]

Created attachment 104368 [details]
Added missing ChangeLog. No other changes.

[James Robinson]

Comment on attachment 104368 [details]
Added missing ChangeLog. No other changes.

Whee changelogs

[WebKit Review Bot]

Comment on attachment 104368 [details]
Added missing ChangeLog. No other changes.

Clearing flags on attachment: 104368

Committed r93346: <http://trac.webkit.org/changeset/93346>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.