Bug 199626 - Fix non thread-safe use of WeakPtr in DisplayRefreshMonitorMac::displayLinkFired()
Summary: Fix non thread-safe use of WeakPtr in DisplayRefreshMonitorMac::displayLinkFi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 199517
  Show dependency treegraph
 
Reported: 2019-07-09 10:49 PDT by Chris Dumez
Modified: 2019-07-09 13:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.73 KB, patch)
2019-07-09 10:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.08 KB, patch)
2019-07-09 11:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (2.33 MB, application/zip)
2019-07-09 11:44 PDT, Build Bot
no flags Details
Patch (8.93 KB, patch)
2019-07-09 11:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-07-09 10:49:21 PDT
Fix non thread-safe use of WeakPtr in DisplayRefreshMonitorMac::displayLinkFired():
Thread 11 Crashed:: CVDisplayLink
0   com.apple.JavaScriptCore      	0x000000010e6a837e WTFCrash + 14 (Assertions.cpp:305)
1   com.apple.WebCore             	0x0000000123e867eb WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x000000012734032c WebCore::DisplayRefreshMonitorMac::WeakValueType* WTF::WeakPtrImpl::get<WebCore::DisplayRefreshMonitorMac>() + 140 (WeakPtr.h:65)
3   com.apple.WebCore             	0x00000001273401aa WTF::WeakPtrFactory<WebCore::DisplayRefreshMonitorMac>::createWeakPtr(WebCore::DisplayRefreshMonitorMac&) const + 138 (WeakPtr.h:142)
4   com.apple.WebCore             	0x000000012733dab7 WTF::WeakPtr<WebCore::DisplayRefreshMonitorMac> WTF::makeWeakPtr<WebCore::DisplayRefreshMonitorMac>(WebCore::DisplayRefreshMonitorMac&) + 55 (WeakPtr.h:212)
5   com.apple.WebCore             	0x000000012733d9ef WebCore::DisplayRefreshMonitorMac::displayLinkFired() + 111 (DisplayRefreshMonitorMac.cpp:92)
6   com.apple.WebCore             	0x000000012733d935 WebCore::displayLinkCallback(__CVDisplayLink*, CVTimeStamp const*, CVTimeStamp const*, unsigned long long, unsigned long long*, void*) + 53 (DisplayRefreshMonitorMac.cpp:53)
7   com.apple.CoreVideo           	0x00007fff3833fe16 CVDisplayLink::performIO(CVTimeStamp*) + 230
8   com.apple.CoreVideo           	0x00007fff3833f2ee CVDisplayLink::runIOThread() + 626
9   libsystem_pthread.dylib       	0x00007fff6dd36daa _pthread_start + 125
10  libsystem_pthread.dylib       	0x00007fff6dd336af thread_start + 15
Comment 1 Chris Dumez 2019-07-09 10:52:33 PDT
Created attachment 373731 [details]
Patch
Comment 2 Build Bot 2019-07-09 10:53:48 PDT
Attachment 373731 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Ryosuke Niwa 2019-07-09 10:56:46 PDT
Comment on attachment 373731 [details]
Patch

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

Maybe we need ThreadSafeWeakPtr...

> Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.h:46
>      void displayLinkFired() override;

This does class need to inherit from CanMakeWeakPtr now?
Comment 4 Chris Dumez 2019-07-09 10:57:29 PDT
(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 373731 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373731&action=review
> 
> Maybe we need ThreadSafeWeakPtr...
> 
> > Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.h:46
> >      void displayLinkFired() override;
> 
> This does class need to inherit from CanMakeWeakPtr now?

Oh, it likely doesn't. Good catch.
Comment 5 Chris Dumez 2019-07-09 11:00:17 PDT
Created attachment 373733 [details]
Patch
Comment 6 Build Bot 2019-07-09 11:02:47 PDT
Attachment 373733 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Build Bot 2019-07-09 11:44:39 PDT
Comment on attachment 373733 [details]
Patch

Attachment 373733 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12699554

Number of test failures exceeded the failure limit.
Comment 8 Build Bot 2019-07-09 11:44:41 PDT
Created attachment 373736 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 9 Chris Dumez 2019-07-09 11:53:12 PDT
Created attachment 373737 [details]
Patch
Comment 10 Build Bot 2019-07-09 11:55:14 PDT
Attachment 373737 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:112:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Chris Dumez 2019-07-09 13:03:12 PDT
Comment on attachment 373737 [details]
Patch

Clearing flags on attachment: 373737

Committed r247273: <https://trac.webkit.org/changeset/247273>
Comment 12 Chris Dumez 2019-07-09 13:03:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-07-09 13:04:21 PDT
<rdar://problem/52848623>