Bug 70161 – [chromium] Fix shutdown race when posting main thread task to CCThreadProxy and enable tests

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Fix shutdown race when posting main thread task to CCThreadProxy and enable tests

[James Robinson]

Created attachment 111116 [details]
Patch

[James Robinson]

Comment on attachment 111116 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCMainThreadThreadProxyProxy.h:36
> +// As should be obvious from the name, this class is a proxy for a CCThreadProxy that can be used to run tasks on the

In case it's not clear the "as should be obvious" part is a bit of a joke.

This class name is accurate (imo) but completely horrible. I think the design pattern is reasonable, so what should I call it?

[Adrienne Walker]

Comment on attachment 111116 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCMainThreadThreadProxyProxy.h:36
>> +// As should be obvious from the name, this class is a proxy for a CCThreadProxy that can be used to run tasks on the
> 
> In case it's not clear the "as should be obvious" part is a bit of a joke.
> 
> This class name is accurate (imo) but completely horrible. I think the design pattern is reasonable, so what should I call it?

CCSafeThreadProxy?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:109
> +    RefPtr<CCMainThreadThreadProxyProxy> m_mttpp;

I'd like to buy a vowel.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:387
> +#define SINGLE_AND_MULTI_THREAD_TEST_F(TEST_FIXTURE_NAME) \
> +    TEST_F(TEST_FIXTURE_NAME, runSingleThread)            \
> +    {                                                     \
> +        runTest(false);                                   \
> +    }                                                     \
> +    TEST_F(TEST_FIXTURE_NAME, runMultiThread)             \
> +    {                                                     \
> +        runTest(true);                                    \
> +    }

Awesome.

[Nat Duca]

Comment on attachment 111116 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCMainThreadThreadProxyProxy.h:36
>>> +// As should be obvious from the name, this class is a proxy for a CCThreadProxy that can be used to run tasks on the
>> 
>> In case it's not clear the "as should be obvious" part is a bit of a joke.
>> 
>> This class name is accurate (imo) but completely horrible. I think the design pattern is reasonable, so what should I call it?
> 
> CCSafeThreadProxy?

So making the proxy threadsaferefcounted didn't pan out?

I think this is ~= Chromium's MessageLoopProxy.

Maybe the right thing to do here is not allow the CCProxy to access CCMainThread at all, or for that matter, CCThread. Rather, it would get access to a MessageLoopProxy, which has a posttask interface. When we delete the proxy, any tasks that were posted via that proxy stop running, even if the underlying message loop lives on.

I'm curious where @levin sits on this.

[James Robinson]

It's not MessageLoopProxy because the target MessageLoop is not going away - just the target of the message.  The closest match I could find in chromium is ScopedRunnableMethodFactory in chromium: http://google.com/codesearch#OAMlx_jo-ck/src/base/task.h&exact_package=chromium&q=ScopedRunnable&l=68 except that this is for use across threads, not for the same thread.  SRMF works by keeping a pointer to each posted task (through WeakPtrFactory) and invalidating them all on shutdown.

[David Levin]

(In reply to comment #4)
> (From update of attachment 111116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111116&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/cc/CCMainThreadThreadProxyProxy.h:36
> >>> +// As should be obvious from the name, this class is a proxy for a CCThreadProxy that can be used to run tasks on the
> >> 
> >> In case it's not clear the "as should be obvious" part is a bit of a joke.
> >> 
> >> This class name is accurate (imo) but completely horrible. I think the design pattern is reasonable, so what should I call it?
> > 
> > CCSafeThreadProxy?
> 
> So making the proxy threadsaferefcounted didn't pan out?
> 
> I think this is ~= Chromium's MessageLoopProxy.
> 
> Maybe the right thing to do here is not allow the CCProxy to access CCMainThread at all, or for that matter, CCThread. Rather, it would get access to a MessageLoopProxy, which has a posttask interface. When we delete the proxy, any tasks that were posted via that proxy stop running, even if the underlying message loop lives on.
> 
> I'm curious where @levin sits on this.

The structure of CCMainThreadThreadProxyProxy looks very similar to a pattern used successfully in a number of places and something I had planned to get around to providing generically but have been distracted by other matters.  (I believe it is similar to the Proxy in http://trac.webkit.org/wiki/ThreadCommunication which is an object proxy).

I would take the stuff this stuff and make it a method on mttpp.
  347    m_mttpp->ref();
   348    return createMainThreadTask(m_mttpp.get(), &CCMainThreadThreadProxyProxy::runTaskIfProxyAlive, createMainThreadTask
It only needs to take a PassOwnPtr<CCMainThread::Task>.

The name on the other hand is terrible. I'm not familiar with all of the classes you have here but it does seem odd to me to have the other Proxy that can't seem to be used as a proxy (and that's where the naming problem starts imo. Also the variable name m_mttpp is not so great either.)

[James Robinson]

The name's mostly a joke, to be honest.  Have a better idea?

[David Levin]

(In reply to comment #7)
> The name's mostly a joke, to be honest.  Have a better idea?

Well, it depends. Look at the rest of the comment. (Basically, I think the current Proxy sounds like a mistake. If that were cleaned up in some way, then this may only have one Proxy in the name. Why is there a "CCThreadProxy" at all? Why not just a CCThread?)

[Nat Duca]

Isn't a generalization of m_mtpp vector<MainThread::Task> that have been posted but not run?

That gets back to my MessageLoopProxy point:
  CCMainThread {
    Task {
    private:
      CCMainThreadProxy* m_proxy; // run() only runs if m_proxy != null;
    }
    OwnPtr<CCMainThreadProxy> getProxy();
  private:
    postTask becomes private

  };
  CCMainThreadProxy {
    void postTask(CCMainThread::Task*) {
       m_pendingTasks->push_back(m_pendingTasks);
       CCMainThread::postTask(t);
    }
    ~CCMainThreadProxy() {
      foreach (t in m_pendingTasks) t->m_proxy = 0;
    }
    vector<Task> m_pendingTasks;
  }
  CCThreadProxy {
    OwnPtr<CCMainThreadProxy> m_mainThreadProxy;
  }

[James Robinson]

Created attachment 111321 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   b98b9c1..915de3d  master     -> origin/master
	M	Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def
	M	Source/JavaScriptCore/ChangeLog
r97648 = df83667e294d8753cdbcdb5c1e4959daeaf988d2 (refs/remotes/trunk)
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/page/Frame.cpp
	M	Source/WebCore/page/DOMWindow.h
	M	Source/WebCore/page/DOMWindow.cpp
	M	Source/WebCore/notifications/NotificationCenter.cpp
	D	Source/WebCore/manual-tests/iframe_notifications/iframe-reparenting-close-window.html
	D	Source/WebCore/manual-tests/iframe_notifications/iframe-reparenting-close-window-child.html
	D	Source/WebCore/manual-tests/iframe_notifications/iframe-reparenting-close-window-iframe.html
W: -empty_dir: trunk/Source/WebCore/manual-tests/iframe_notifications/iframe-reparenting-close-window-child.html
W: -empty_dir: trunk/Source/WebCore/manual-tests/iframe_notifications/iframe-reparenting-close-window-iframe.html
W: -empty_dir: trunk/Source/WebCore/manual-tests/iframe_notifications/iframe-reparenting-close-window.html
r97649 = 955471c065449b8ccf2b5969f57528d4f33a4a0e (refs/remotes/trunk)
	M	Source/JavaScriptCore/ChangeLog
	M	Source/JavaScriptCore/JavaScriptCore.exp
r97650 = 915de3d57db2858b468fb876de4b172fea01f51a (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


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

[James Robinson]

This patch calls it a CancelableTaskFactory and hides all the ref/deref stuff internally. It does the same thing as #9, I believe, but instead of having to keep track of all of the tasks it thunks tasks through itself on the main thread to check for a cancellation flag. PTAL.

[James Robinson]

Created attachment 111327 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


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

[Nat Duca]

Comment on attachment 111327 [details]
Patch

Nice!

I think the naming needs some love. When I read this, I assumed from the name that tasks were individually cancellable.

I agree that's overkill. So the right name and I like

[James Robinson]

Comment on attachment 111327 [details]
Patch

Clearing review for now, Nat had some good feedback out-of-band. I'll address that and then post a revised patch.

[James Robinson]

Created attachment 111334 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


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

[James Robinson]

Created attachment 111350 [details]
Patch

[James Robinson]

New name for great justice! This time it's a CCScopedMainThreadProxy and it's the only way you are allowed to post tasks to the main thread.  Shutting down the proxy (via ->shutdown()) cancels all tasks posted through the proxy.

[James Robinson]

One more note: This one's a real proxy in the way we usually use it (like MessageLoopProxy) in that it's a thing you use instead of the real CCMainThread::postTask().

[David Levin]

With a cursory look, it seems fine to me. It would be nice for people more familiar with the area to look at it as well.

[David Levin]

Comment on attachment 111350 [details]
Patch

r+ but it would be great if you got one more set of eyes on it as well.

[James Robinson]

Nat promised to take another look, so I'll wait for an unofficial review from him as well before landing. Thanks for reviewing!

[Nat Duca]

Comment on attachment 111350 [details]
Patch

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

Unofficially, looks good. Thank you James, and my apologies for not having caught this earlier on. :$

> Source/WebCore/platform/graphics/chromium/cc/CCMainThread.h:34
>  // Task wrapper around WTF::callOnMainThreadThread

Put comment here? Something like "if you wanna post a task to the main thread, go look at ScopedmainThreadproxy." But with pristine punctuation and english. ;)

> Source/WebCore/platform/graphics/chromium/cc/CCScopedMainThreadProxy.h:37
> +//

Might make a note on rationale on why one uses this. E.g. "Use this when you are posting a message against an object that might die between the time you post the task and it executes.

[James Robinson]

Committed r97690: <http://trac.webkit.org/changeset/97690>

[James Robinson]

Reverted r97690 for reason:

Hits assertion in CCLayerTreeHostTests

Committed r97692: <http://trac.webkit.org/changeset/97692>

[James Robinson]

The tests popped because CCProxy::isMainThread() has gotten pickier recently.  The fix for that is easy.

However, I've also noticed that a lot of tests are actually flaky.

[James Robinson]

Created attachment 111479 [details]
Patch against attachment #5 that fixes flake and main thread ASSERT

[James Robinson]

The ASSERT() was because I wasn't setting the main thread ID before constructing the CCScopedMainThreadProxy and CCProxy::isMainThread() is now stricter about checking this.

The flake was because we were immediately triggering an asynchronous redraw and commit when constructing a LayerTreeHost and these tasks would sometimes run before the actual test code had a chance to run.

[Nat Duca]

I know I put that setNeedsCommitAndRedraw in the constructor for a reason, but I can't see any problem with removing it. That may have been left over from the first 2-stage-commit implementation.

Long winded way of me saying "unofficially looks good."

[James Robinson]

Committed r97784: <http://trac.webkit.org/changeset/97784>