WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
70161
[chromium] Fix shutdown race when posting main thread task to CCThreadProxy and enable tests
https://bugs.webkit.org/show_bug.cgi?id=70161
Summary
[chromium] Fix shutdown race when posting main thread task to CCThreadProxy a...
James Robinson
Reported
2011-10-14 17:45:19 PDT
[chromium] Fix shutdown race when posting main thread task to CCThreadProxy and enable tests
Attachments
Patch
(18.94 KB, patch)
2011-10-14 17:50 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(18.82 KB, patch)
2011-10-17 14:29 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(18.31 KB, patch)
2011-10-17 15:09 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(19.86 KB, patch)
2011-10-17 15:45 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(23.82 KB, patch)
2011-10-17 16:56 PDT
,
James Robinson
levin
: review+
Details
Formatted Diff
Diff
Patch against attachment #5 that fixes flake and main thread ASSERT
(1.55 KB, patch)
2011-10-18 12:18 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
James Robinson
Comment 1
2011-10-14 17:50:55 PDT
Created
attachment 111116
[details]
Patch
James Robinson
Comment 2
2011-10-14 17:52:47 PDT
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 3
2011-10-17 09:00:46 PDT
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 4
2011-10-17 10:32:26 PDT
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
Comment 5
2011-10-17 10:37:17 PDT
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
Comment 6
2011-10-17 10:45:14 PDT
(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
Comment 7
2011-10-17 10:48:28 PDT
The name's mostly a joke, to be honest. Have a better idea?
David Levin
Comment 8
2011-10-17 10:51:18 PDT
(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
Comment 9
2011-10-17 11:52:40 PDT
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
Comment 10
2011-10-17 14:29:32 PDT
Created
attachment 111321
[details]
Patch
WebKit Review Bot
Comment 11
2011-10-17 14:31:05 PDT
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
Comment 12
2011-10-17 14:34:08 PDT
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
Comment 13
2011-10-17 15:09:49 PDT
Created
attachment 111327
[details]
Patch
WebKit Review Bot
Comment 14
2011-10-17 15:13:20 PDT
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 15
2011-10-17 15:13:58 PDT
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 16
2011-10-17 15:18:01 PDT
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
Comment 17
2011-10-17 15:45:24 PDT
Created
attachment 111334
[details]
Patch
WebKit Review Bot
Comment 18
2011-10-17 15:47:54 PDT
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
Comment 19
2011-10-17 16:56:33 PDT
Created
attachment 111350
[details]
Patch
James Robinson
Comment 20
2011-10-17 16:57:23 PDT
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
Comment 21
2011-10-17 16:57:59 PDT
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
Comment 22
2011-10-17 17:27:59 PDT
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 23
2011-10-17 17:29:23 PDT
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
Comment 24
2011-10-17 17:40:53 PDT
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 25
2011-10-17 17:53:09 PDT
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
Comment 26
2011-10-17 18:14:17 PDT
Committed
r97690
: <
http://trac.webkit.org/changeset/97690
>
James Robinson
Comment 27
2011-10-17 18:23:05 PDT
Reverted
r97690
for reason: Hits assertion in CCLayerTreeHostTests Committed
r97692
: <
http://trac.webkit.org/changeset/97692
>
James Robinson
Comment 28
2011-10-18 12:16:26 PDT
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
Comment 29
2011-10-18 12:18:13 PDT
Created
attachment 111479
[details]
Patch against attachment #5 that fixes flake and main thread ASSERT
James Robinson
Comment 30
2011-10-18 12:20:01 PDT
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
Comment 31
2011-10-18 12:48:04 PDT
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
Comment 32
2011-10-18 12:54:28 PDT
Committed
r97784
: <
http://trac.webkit.org/changeset/97784
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug