Bug 226157 – Make CheckedLock the default Lock

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Make CheckedLock the default Lock.

Rename Lock to UncheckedLock, CheckedLock to Lock. Also rename Condition to UncheckedCondition and CheckedCondition to Condition.

[Darin Adler]

Safer to do this in separate steps. A single patch where Lock is valid both before and after but means two different things is much more risky.

Also, should the Unchecked ones exist long term, or are they deprecated and slated for destruction? If the latter, then I suggest names with Deprecated in them.

[Chris Dumez]

(In reply to Darin Adler from comment #1)
> Safer to do this in separate steps. A single patch where Lock is valid both
> before and after but means two different things is much more risky.

The issue is that not ALL existing Lock cases will be renamed to UncheckedLock. As a matter of fact, most of them will just build fine as is when the UncheckedLock become the Lock.

The approach I used was doing the rename of the Locks, and then only update variables from Lock to UncheckedLock if I get a build failure.

I am not sure I understand the risk you're referring to here. Either it builds or it doesn't. The locks are identical at run time.

> Also, should the Unchecked ones exist long term, or are they deprecated and
> slated for destruction? If the latter, then I suggest names with Deprecated
> in them.

I am proposing to start with UncheckedLock for now and see how it goes. If we see that things go well and we managed to get to low usage then add deprecated to the name later on. There are cases that rely on Locker having a move constructor, in particular in JavaScriptCore and this is something that Locker<CheckedLock> cannot support for e.g.

[Chris Dumez]

By the way, the reason I landed so many patches before this one ( to get rid of holdLock() / tryHoldLock and LockHolder variables) is because as a result, most existing Locks will build just fine as a result once I rename CheckedLock to Lock.

The cases that will need to be renamed to UncheckedLock are mostly cases where lockers are moved and passed as parameter as AbstractLocker. And Lockers that take in a pointer to a lock (instead of a reference). With some work though, we can likely factor the code a bit differently to get rid of UncheckedLock I hope.

[Darin Adler]

(In reply to Chris Dumez from comment #2)
> I am not sure I understand the risk you're referring to here. Either it
> builds or it doesn't. The locks are identical at run time.

OK. I guess this is an unusual case where it’s completely safe to get it wrong!

[Chris Dumez]

Created attachment 429478 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Chris Dumez]

Created attachment 429479 [details]
Patch

[Chris Dumez]

Created attachment 429482 [details]
Patch

[Chris Dumez]

Created attachment 429484 [details]
Patch

[Chris Dumez]

Created attachment 429486 [details]
Patch

[Chris Dumez]

Created attachment 429487 [details]
Patch

[Chris Dumez]

Created attachment 429488 [details]
Patch

[Chris Dumez]

Created attachment 429493 [details]
Patch

[Chris Dumez]

I am preparing an explanation email for Clang thread safety analysis in WebKit and the related annotations for webkit-dev. This way, people will know our to leverage this in new code or how to deal with compilation errors when they modify the code. I'll send out the mail after this lands.

[Darin Adler]

Comment on attachment 429493 [details]
Patch

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

Seems like a good start. I couldn’t really discern the pattern of where we use UncheckedLock explicitly in this patch, even though you explained the strategy and rationale, but I suppose the main point is switching to the checking from CheckedLock almost everywhere that is *not* in the patch.

> Source/WTF/wtf/Lock.h:216
> +using WTF::assertIsHeld;

Just occurred to me that we might not need this because of argument dependent lookup <https://en.cppreference.com/w/cpp/language/adl>. If the argument is in the WTF namespace, then the function is looked for in the WTF namespace without requiring a WTF:: prefix, so this using line solves a problem that does not exist.

> Source/WebCore/html/canvas/WebGLObject.h:31
> +#include <wtf/Lock.h>

Could we use Forward.h here instead, and remove the forward declaration of AbstractLocker below?

[Chris Dumez]

(In reply to Darin Adler from comment #15)
> Comment on attachment 429493 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429493&action=review
> 
> Seems like a good start. I couldn’t really discern the pattern of where we
> use UncheckedLock explicitly in this patch, even though you explained the
> strategy and rationale, but I suppose the main point is switching to the
> checking from CheckedLock almost everywhere that is *not* in the patch.
> 
> > Source/WTF/wtf/Lock.h:216
> > +using WTF::assertIsHeld;
> 
> Just occurred to me that we might not need this because of argument
> dependent lookup <https://en.cppreference.com/w/cpp/language/adl>. If the
> argument is in the WTF namespace, then the function is looked for in the WTF
> namespace without requiring a WTF:: prefix, so this using line solves a
> problem that does not exist.

Will try.

> 
> > Source/WebCore/html/canvas/WebGLObject.h:31
> > +#include <wtf/Lock.h>
> 
> Could we use Forward.h here instead, and remove the forward declaration of
> AbstractLocker below?

I think this means adding those forward declarations to Forward.h since they are not currently present.

[Darin Adler]

Comment on attachment 429493 [details]
Patch

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

>>> Source/WebCore/html/canvas/WebGLObject.h:31
>>> +#include <wtf/Lock.h>
>> 
>> Could we use Forward.h here instead, and remove the forward declaration of AbstractLocker below?
> 
> I think this means adding those forward declarations to Forward.h since they are not currently present.

We should do that eventually, even if we don’t do it right now.

[Chris Dumez]

Created attachment 429499 [details]
Patch

[EWS]

Committed r277943 (238070@main): <https://commits.webkit.org/238070@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429499 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78385912>