Bug 225228 - CheckedLock::tryLock() use pattern should be more robust
Summary: CheckedLock::tryLock() use pattern should be more robust
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-30 05:55 PDT by Kimmo Kinnunen
Modified: 2021-06-14 02:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.97 KB, patch)
2021-04-30 06:23 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-04-30 05:55:53 PDT
CheckedLock::tryLock() use pattern should be more robust
From bug 224970 discussions
Comment 1 Kimmo Kinnunen 2021-04-30 06:21:27 PDT
Bug 224970
(In reply to Chris Dumez from comment #15)
> 
> My preference would have been:
> TryLocker locker(lock);
> if (!locker.isLocked())
>   return;
> 
> This is also what Blink uses.
> 
> I dislike any kind of adopted lock. I don't think we should be interacting
> with the lock directly or adopting lock as this is a recipe for mistakes.

Blink MutexTryLocker does not support thread safety analysis.
As such it is equivalent of already implemented auto locker = tryHoldLock().

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/threading_primitives.h



I failed to write a TryLocker that would support current implementation of thread safety analysis.

Suppose we set aside the preferences for syntactical constructs for some time.



The decision of what syntax to use wrt tryLock is based on which problem are you solving:

A) The problem to solve is that caller forgets to adopt the lock after tryLock().
B) The problem to solve is that caller accesses the locked variables without lock.

My (relatively small) experience leads to think that B is the more important problem to solve.

In patch below I'm trying to communicate that the thread safety analysis already covers all of B and most of A.
Comment 2 Kimmo Kinnunen 2021-04-30 06:23:17 PDT
Created attachment 427412 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2021-05-07 05:56:13 PDT
<rdar://problem/77653579>