Bug 223222 – [Cocoa] Update legacy WebKit thread checks to prepare for use on iOS and modern WebKit

• Status: ASSIGNED
• Component: WebKit API

[Darin Adler]

[Cocoa] Update legacy WebKit thread checks to prepare for use on iOS and modern WebKit

[Darin Adler]

Created attachment 423263 [details]
Patch

[Chris Dumez]

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 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]

I wonder if we can write API tests for these...

[Darin Adler]

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 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]

<rdar://problem/75714190>