Bug 226281 – Stop using UncheckedLock in JSDOMGlobalObject

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Stop using UncheckedLock in JSDOMGlobalObject as it is being phased out in favor of Lock, which supports Clang thread safety analysis.

[Chris Dumez]

Created attachment 429775 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 429775 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Clang thread safety analysis.

It's not clear if the payload of the commit is to
a) Prove `JSDOMGlobalObject` locking more correct
b) Reducing the usage of `UncheckedLock` with the goal of eventually removing `UncheckedLock` on the grounds of "less classes, more simplicity".

I think `UncheckedLock` is pretty good class for its purpose: it allows `Lock` use sites to be proven more correct by providing an escape hatch for wild behavior using UncheckedLock. (However, some code has already gone in Lock that decreases its usefulness from this perspective.)

From this viewpoint, I think a) is a worthy goal while b) is not, if b) would mean reducing the proof power of `Lock`.

This is on the grounds that if we merge features of `CheckedLock` and `UncheckedLock` into one amalgamation of `Lock`, then it might be that we lose the ability to prove useful aspects or at least it may happen that we _think_ we are proving something when in reality it is easy for all but non-experts to invoke the escape hatch aspects of the implementation by mistake.

> Source/WTF/wtf/Lock.h:195
> +    Locker(Locker&& other) // Not compatible with Clang thread safety analysis.

I would find clang more consistent if it complained about acquire/release mismatch when running the destructor. Unsure if it's just unimplemented or by design..

I'm not sure the Lock is moving in the right direction..

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:121
>  

To have a payload a) in this commit I'd imagine you'd do following:
```
   JSDOMStructureMap m_structures WTF_GUARDED_BY(m_gcLock);
   ...
```

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:124
> +    Lock m_gcLock;

```
   GCLock m_gcLock;
```

And then you'd have :

```
class WTF_CAPABILITY_LOCK GCLock { ... };
class WTF_CAPABILITY_SCOPED_LOCK GCLocker { ... };

GCLocker { vm.heap, mutableGlobalObject };
mutableGlobalObject.constructors().add(ConstructorClass::info(), temp).iterator->value.set(vm, &globalObject, constructor);
...
```

So:
1) remove the anti-pattern of passing the locker
2) Change viewpoint
  "Object should be locked if heap says mutator should be fenced. An operation can be done to the object with or without the lock, but be sure to know what you're doing."
  to
 "Object should be always locked and only then the operation can be done to the object. The object lock is a special case lock in the way that it does something if heap says mutator should be fenced, otherwise it's a no-op.".

> Source/WebCore/bindings/js/JSDOMGuardedObject.cpp:40
> +    if (locker)

so this would be:
```
GCLocker { globalObject.vm().heap, globalObject };
globalObject.guardedObjects().add(this);
```

[Kimmo Kinnunen]

>easy for all but non-experts to invoke the escape hatch aspects of the implementation by mistake.

all but *experts* ..

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #2)
> Comment on attachment 429775 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429775&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Clang thread safety analysis.
> 
> It's not clear if the payload of the commit is to
> a) Prove `JSDOMGlobalObject` locking more correct
> b) Reducing the usage of `UncheckedLock` with the goal of eventually
> removing `UncheckedLock` on the grounds of "less classes, more simplicity".

Ideally, a) would be better. However, it is not really possible here. Of course, I am definitely going for b).
Per previously received feedback, I am working to removed UncheckedLock from the code base so that we have a single Lock class.
And by the way, I am almost done (2 small commits away). Yes, having a single Lock adds simplicity.

There are many cases where we can get at least partial validation using Lock, even if the code does something the analysis doesn't support is some functions and those functions are marked with WTF_IGNORES_THEAD_SAFETY_ANALYSIS. Using a particular Lock type is infectious. By that I mean that if I use it in one class, I may have to use with in many other classes to get things to build
properly (WTF::AutomaticThread was a good example example of that. JSC had to use UncheckedLock because AutomaticThread and vice-versa).

> 
> I think `UncheckedLock` is pretty good class for its purpose: it allows
> `Lock` use sites to be proven more correct by providing an escape hatch for
> wild behavior using UncheckedLock. (However, some code has already gone in
> Lock that decreases its usefulness from this perspective.)

The issue really isn't with the Lock implementation though. All the code could be using Lock without trouble, albeit using WTF_IGNORES_THEAD_SAFETY_ANALYSIS in some places.
The issue is the the Locker<Lock> specialization is super inflexible and doesn't allow me to do things that I should be able to do.

I don't think we want a escape hatch. I think we want the code to get the benefits of analysis as much as possible, and in cases where analysis is not supported, use WTF_IGNORES_THEAD_SAFETY_ANALYSIS.
Note that there are already cases with Locker where analysis is not supported and you have to use WTF_IGNORES_THEAD_SAFETY_ANALYSIS. Using Locker<Lock> does not guarantee you'll get thread safety analysis, e.g.:
```
if (foo) {
    Locker lock { m_lock }
    // Do something while locked.
}
// Do something while unlocked
```
or using an Optional<Locker<Lock>>.

We also don't want to have people use the UncheckedLock escape hatch simply because our Locker implementation is to inconvenient to use (e.g. I cannot return a Locker<Lock> from a function).

> 
> From this viewpoint, I think a) is a worthy goal while b) is not, if b)
> would mean reducing the proof power of `Lock`.
> 
> This is on the grounds that if we merge features of `CheckedLock` and
> `UncheckedLock` into one amalgamation of `Lock`, then it might be that we
> lose the ability to prove useful aspects or at least it may happen that we
> _think_ we are proving something when in reality it is easy for all but
> non-experts to invoke the escape hatch aspects of the implementation by
> mistake.
> 
> > Source/WTF/wtf/Lock.h:195
> > +    Locker(Locker&& other) // Not compatible with Clang thread safety analysis.
> 
> I would find clang more consistent if it complained about acquire/release
> mismatch when running the destructor. Unsure if it's just unimplemented or
> by design..
> 
> I'm not sure the Lock is moving in the right direction..
> 
> > Source/WebCore/bindings/js/JSDOMGlobalObject.h:121
> >  
> 
> To have a payload a) in this commit I'd imagine you'd do following:
> ```
>    JSDOMStructureMap m_structures WTF_GUARDED_BY(m_gcLock);
>    ...
> ```
> 
> > Source/WebCore/bindings/js/JSDOMGlobalObject.h:124
> > +    Lock m_gcLock;
> 
> ```
>    GCLock m_gcLock;
> ```
> 
> And then you'd have :
> 
> ```
> class WTF_CAPABILITY_LOCK GCLock { ... };
> class WTF_CAPABILITY_SCOPED_LOCK GCLocker { ... };
> 
> GCLocker { vm.heap, mutableGlobalObject };
> mutableGlobalObject.constructors().add(ConstructorClass::info(),
> temp).iterator->value.set(vm, &globalObject, constructor);
> ...
> ```
> 
> So:
> 1) remove the anti-pattern of passing the locker

I don't think it is an anti-pattern in those cases though. JSC has the pattern of passing an AbstractLocker (Not a Locker<>) around for functions that usually require a lock to be acquired first.
HOWEVER, there are cases where they know it is safe to run without locking. In such cases, they pass an AbstractLocker { NoLockingNecessary }. It documents that locking is usually necessary and forces call sites to think about locking. It also supports being efficient in cases where we know Locking is pointless (because no other thread is running at the moment for e.g.) I personally see how this pattern can be
useful. I also see how it is incompatible with Clang thread safety validation. However, I disagree that this means that every code using this pattern cannot use the Lock type and has to
use UncheckedLock as a result.

> 2) Change viewpoint
>   "Object should be locked if heap says mutator should be fenced. An
> operation can be done to the object with or without the lock, but be sure to
> know what you're doing."
>   to
>  "Object should be always locked and only then the operation can be done to
> the object. The object lock is a special case lock in the way that it does
> something if heap says mutator should be fenced, otherwise it's a no-op.".
> 
> > Source/WebCore/bindings/js/JSDOMGuardedObject.cpp:40
> > +    if (locker)
> 
> so this would be:
> ```
> GCLocker { globalObject.vm().heap, globalObject };
> globalObject.guardedObjects().add(this);
> ```

In the end, I think we want code to benefit from analysis as possible and I think we also want the WebKit project to be as easily hackable as possible. Having a single Lock and a convenient Locker<> go a long way towards the simplicity goal. Having a single Lock type that supports analysis go a long way towards getting as much validation as possible. Sure, there will always be cases where we have to do (or decide to do) things that are not supported by clang analysis, that is fine but the scope should be as limited as possible.

[Chris Dumez]

Comment on attachment 429775 [details]
Patch

I will see what I can do about getting more benefits from analysis in this patch. To be clear though, I 100% think we should get rid of UncheckedLock.

[Chris Dumez]

Created attachment 429882 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Created attachment 429882 [details]
> Patch

This should be a lot less controversial. I did not introduce a new type of lock / locker though as this is the opposite direction I want to go to and also, there would be very few users.

[Chris Dumez]

Created attachment 429885 [details]
Patch

[Chris Dumez]

Created attachment 429890 [details]
Patch

[Chris Dumez]

Created attachment 429894 [details]
Patch

[Chris Dumez]

Created attachment 429897 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 429897 [details]
Patch

From the perspective of changing the lock type, makes sense to me.

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #12)
> Comment on attachment 429897 [details]
> Patch
> 
> From the perspective of changing the lock type, makes sense to me.

Patch is ready for review then.

[Chris Dumez]

Anybody willing to review? I don't think the patch is controversial anymore as it doesn't change WTF::Lock and it adds a lot of clang analysis validation to the JSDOMGlobalObject code.

I am super close to having no use of UncheckedLock in WebKit.

[Darin Adler]

Comment on attachment 429897 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:259
> +        // The GC thread has to grab the gc lock even though it is not mutating the containers.

I would capitalize "gc" both times.

[Darin Adler]

Comment on attachment 429897 [details]
Patch

Could we have avoided all the duplicate code using std::optional<Locker> or would that not work with the clang analysis?

[Chris Dumez]

(In reply to Darin Adler from comment #16)
> Comment on attachment 429897 [details]
> Patch
> 
> Could we have avoided all the duplicate code using std::optional<Locker> or
> would that not work with the clang analysis?

std::optional<Locker> does build but then you get no clang thread safety analysis. Analysis does not support "conditional" locking and thus forcing us to use branches like I did if we want the validation.

[Chris Dumez]

Committed r278229 (238266@main): <https://commits.webkit.org/238266@main>

[Chris Dumez]

(In reply to Chris Dumez from comment #17)
> (In reply to Darin Adler from comment #16)
> > Comment on attachment 429897 [details]
> > Patch
> > 
> > Could we have avoided all the duplicate code using std::optional<Locker> or
> > would that not work with the clang analysis?
> 
> std::optional<Locker> does build but then you get no clang thread safety
> analysis. Analysis does not support "conditional" locking and thus forcing
> us to use branches like I did if we want the validation.

When I said it would build, I meant with WTF_IGNORES_THREAD_SAFETY_ANALYSIS.

[Radar WebKit Bug Importer]

<rdar://problem/78637376>