[chromium] Fix shutdown race when posting main thread task to CCThreadProxy and enable tests
Created attachment 111116 [details] Patch
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?
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.
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.
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.
(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.)
The name's mostly a joke, to be honest. Have a better idea?
(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?)
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; }
Created attachment 111321 [details] Patch
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.
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.
Created attachment 111327 [details] Patch
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.
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
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.
Created attachment 111334 [details] Patch
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.
Created attachment 111350 [details] Patch
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.
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().
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.
Comment on attachment 111350 [details] Patch r+ but it would be great if you got one more set of eyes on it as well.
Nat promised to take another look, so I'll wait for an unofficial review from him as well before landing. Thanks for reviewing!
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.
Committed r97690: <http://trac.webkit.org/changeset/97690>
Reverted r97690 for reason: Hits assertion in CCLayerTreeHostTests Committed r97692: <http://trac.webkit.org/changeset/97692>
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.
Created attachment 111479 [details] Patch against attachment #5 that fixes flake and main thread ASSERT
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.
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."
Committed r97784: <http://trac.webkit.org/changeset/97784>