Stop using UncheckedLock in JSDOMGlobalObject as it is being phased out in favor of Lock, which supports Clang thread safety analysis.
Created attachment 429775 [details] Patch
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); ```
>easy for all but non-experts to invoke the escape hatch aspects of the implementation by mistake. all but *experts* ..
(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.
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.
Created attachment 429882 [details] Patch
(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.
Created attachment 429885 [details] Patch
Created attachment 429890 [details] Patch
Created attachment 429894 [details] Patch
Created attachment 429897 [details] Patch
Comment on attachment 429897 [details] Patch From the perspective of changing the lock type, makes sense to me.
(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.
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.
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.
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?
(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.
Committed r278229 (238266@main): <https://commits.webkit.org/238266@main>
(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.
<rdar://problem/78637376>