WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
ASSIGNED
223222
[Cocoa] Update legacy WebKit thread checks to prepare for use on iOS and modern WebKit
https://bugs.webkit.org/show_bug.cgi?id=223222
Summary
[Cocoa] Update legacy WebKit thread checks to prepare for use on iOS and mode...
Darin Adler
Reported
2021-03-15 16:25:48 PDT
[Cocoa] Update legacy WebKit thread checks to prepare for use on iOS and modern WebKit
Attachments
Patch
(127.94 KB, patch)
2021-03-15 16:57 PDT
,
Darin Adler
cdumez
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
2021-03-15 16:57:50 PDT
Created
attachment 423263
[details]
Patch
Chris Dumez
Comment 2
2021-03-16 12:41:57 PDT
Comment on
attachment 423263
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=423263&action=review
Looks good to me.
> Source/WebCore/platform/mac/ThreadCheck.mm:74 > + NSLog(@"WebKit Threading Violation - %s was called from secondary thread", function);
Maybe we should be using RELEASE_LOG_ERROR() so that its stands out in console logging as an error?
> Source/WebCore/platform/mac/ThreadCheck.mm:75 > + NSLog(@"Additional threading violations for this function will not be logged.");
ditto.
> Source/WebCore/platform/mac/ThreadCheck.mm:78 > + NSLog(@"WebKit Threading Violation - %s was called from secondary thread", function);
Ditto.
Ryosuke Niwa
Comment 3
2021-03-16 12:47:02 PDT
Comment on
attachment 423263
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=423263&action=review
> Source/WebCore/platform/ThreadCheck.h:40 > + using BehaviorArray = std::array<Behavior, std::numeric_limits<uint8_t>::max()>;
It might be better to have a single global round / behavior versions so that there is no confusion about "which round one" we're talking about.
> Source/WebCore/platform/ThreadCheck.h:53 > + static std::once_flag flag; > + std::call_once(flag, [] { > + initializeBehaviors(setDefaultBehaviors); > + });
Hm... I think it would be more clear to have this code in the constructor of ThreadViolationChecker.
Ryosuke Niwa
Comment 4
2021-03-16 12:50:17 PDT
I wonder if we can write API tests for these...
Darin Adler
Comment 5
2021-03-16 14:04:01 PDT
Comment on
attachment 423263
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=423263&action=review
>> Source/WebCore/platform/ThreadCheck.h:53 >> + }); > > Hm... I think it would be more clear to have this code in the constructor of ThreadViolationChecker.
The reason it’s here is so that the caller can pass in the function as a template argument. I didn’t want the class itself to be a class template.
Darin Adler
Comment 6
2021-03-16 14:05:35 PDT
Comment on
attachment 423263
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=423263&action=review
>> Source/WebCore/platform/ThreadCheck.h:40 >> + using BehaviorArray = std::array<Behavior, std::numeric_limits<uint8_t>::max()>; > > It might be better to have a single global round / behavior versions > so that there is no confusion about "which round one" we're talking about.
The design here is that WebKit has a set of rounds, and WebKitLegacy has a separate set. That’s why each object has a different behavior array. One for WebKit and one for WebKitLegacy. I thought it would be clearer to keep them separate, since they are based on different linked-on-or-after checks. Could go another way, of course.
Radar WebKit Bug Importer
Comment 7
2021-03-22 16:26:14 PDT
<
rdar://problem/75714190
>
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