Bug 148089

Summary: Replace all remaining uses of WTF::Mutex with WTF::Lock
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: Web Template FrameworkAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, basile_clement, benjamin, buildbot, commit-queue, ggaren, mark.lam, mhahnenb, mmirman, msaboff, nrotem, oliver, rniwa, sam
Priority: P2    
Version: Other   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 147841    
Attachments:
Description Flags
work in progress
none
the patch
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
the patch
none
the patch
none
the patch
none
the patch
none
the patch
none
the patch
none
fix even more builds
none
the patch ggaren: review+

Description Filip Pizlo 2015-08-17 13:27:33 PDT 
And replace WTF::ThreadCondition with WTF::Condition.
Comment 1 Filip Pizlo 2015-08-17 14:45:56 PDT 
Created attachment 259190 [details]
work in progress
Comment 2 Filip Pizlo 2015-08-17 16:03:10 PDT 
Created attachment 259197 [details]
the patch

I plan to add some more things to this patch, like some better tests and benchmarks for the new Condition functionality.  But right now this is passing all of our existing tests, which are pretty comprehensive.
Comment 3 Build Bot 2015-08-17 16:52:21 PDT 
Comment on attachment 259197 [details]
the patch

Attachment 259197 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/69504

Number of test failures exceeded the failure limit.
Comment 4 Build Bot 2015-08-17 16:52:25 PDT 
Created attachment 259207 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 5 Build Bot 2015-08-17 16:57:37 PDT 
Comment on attachment 259197 [details]
the patch

Attachment 259197 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/69508

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2015-08-17 16:57:41 PDT 
Created attachment 259210 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 7 Filip Pizlo 2015-08-17 19:21:02 PDT 
Comment on attachment 259197 [details]
the patch

Clearing r? because EWS reports test crashes
Comment 8 Filip Pizlo 2015-08-17 19:26:34 PDT 
(In reply to comment #7)
> Comment on attachment 259197 [details]
> the patch
> 
> Clearing r? because EWS reports test crashes

Fixed locally.  It was obvious when I tested debug.
Comment 9 Filip Pizlo 2015-08-17 20:37:24 PDT 
Created attachment 259232 [details]
the patch

Fixes crashes in the last patch.  Adds a speed test.
Comment 10 WebKit Commit Bot 2015-08-17 20:39:18 PDT 
Attachment 259232 [details] did not pass style-queue:


ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 12 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Filip Pizlo 2015-08-17 21:18:08 PDT 
Created attachment 259236 [details]
the patch

Fix some builds.
Comment 12 WebKit Commit Bot 2015-08-17 21:20:57 PDT 
Attachment 259236 [details] did not pass style-queue:


ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 12 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Filip Pizlo 2015-08-17 21:33:25 PDT 
Created attachment 259239 [details]
the patch

Another build fix.
Comment 14 WebKit Commit Bot 2015-08-17 21:34:56 PDT 
Attachment 259239 [details] did not pass style-queue:


ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 12 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Filip Pizlo 2015-08-17 22:15:06 PDT 
Created attachment 259246 [details]
the patch

More build fixes for Linux, iOS, and Windows.
Comment 16 WebKit Commit Bot 2015-08-17 22:17:56 PDT 
Attachment 259246 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 13 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Filip Pizlo 2015-08-17 22:35:39 PDT 
Created attachment 259249 [details]
the patch

More build fixes.
Comment 18 WebKit Commit Bot 2015-08-17 22:38:12 PDT 
Attachment 259249 [details] did not pass style-queue:


ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 12 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Filip Pizlo 2015-08-17 22:41:58 PDT 
Created attachment 259250 [details]
the patch

Fix more things
Comment 20 WebKit Commit Bot 2015-08-17 22:45:31 PDT 
Attachment 259250 [details] did not pass style-queue:


ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 12 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Filip Pizlo 2015-08-17 23:18:35 PDT 
Created attachment 259252 [details]
fix even more builds
Comment 22 WebKit Commit Bot 2015-08-17 23:20:26 PDT 
Attachment 259252 [details] did not pass style-queue:


ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 12 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Filip Pizlo 2015-08-17 23:46:27 PDT 
Created attachment 259254 [details]
the patch

Bunch more Windows fixes.
Comment 24 WebKit Commit Bot 2015-08-17 23:47:56 PDT 
Attachment 259254 [details] did not pass style-queue:


ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 12 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Geoffrey Garen 2015-08-18 10:14:53 PDT 
Comment on attachment 259254 [details]
the patch

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

r=me

> Source/WTF/wtf/Condition.h:66
> +    // May wait too long on some OSes: the C++ standard library implementation of
> +    // std::condition_variable::wait_until(), which this uses internally, may convert the absolute
> +    // time to an absolute time according to the system clock, which isn't monotonic. It will do this
> +    // so that it can use pthread_cond_timedwait(). Depending on the OS and if an unfortunate
> +    // combination of thread interleavings and system time changes happens, this may cause waitUntil()
> +    // to wait longer than it's supposed to. However, it's typical for OSes to turn the absolute
> +    // timeout in pthread_cond_timedwait() to a relative timeout before calling into the kernel. This
> +    // is what glibc does, for example.

This seems more like documentation of an OS bug than of an intended quirk of the design here. Perhaps it doesn't belong here.
Comment 26 Filip Pizlo 2015-08-18 12:20:10 PDT 
(In reply to comment #25)
> Comment on attachment 259254 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259254&action=review
> 
> r=me
> 
> > Source/WTF/wtf/Condition.h:66
> > +    // May wait too long on some OSes: the C++ standard library implementation of
> > +    // std::condition_variable::wait_until(), which this uses internally, may convert the absolute
> > +    // time to an absolute time according to the system clock, which isn't monotonic. It will do this
> > +    // so that it can use pthread_cond_timedwait(). Depending on the OS and if an unfortunate
> > +    // combination of thread interleavings and system time changes happens, this may cause waitUntil()
> > +    // to wait longer than it's supposed to. However, it's typical for OSes to turn the absolute
> > +    // timeout in pthread_cond_timedwait() to a relative timeout before calling into the kernel. This
> > +    // is what glibc does, for example.
> 
> This seems more like documentation of an OS bug than of an intended quirk of
> the design here. Perhaps it doesn't belong here.

Fair enough, removed!
Comment 27 Filip Pizlo 2015-08-18 12:32:09 PDT 
Landed in http://trac.webkit.org/changeset/188594