Bug 224970 – Add a Condition type that supports thread safety analysis

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

[Kimmo Kinnunen]

Add a Condition type that supports thread safety analysis
Compile should fail if that condition is waited without holding the corresponding lock.

[Kimmo Kinnunen]

Created attachment 426890 [details]
Patch

[Darin Adler]

Comment on attachment 426890 [details]
Patch

Even better if we found one place to us this in non-test code and included that in this initial patch.

[Darin Adler]

use

[Kimmo Kinnunen]

Created attachment 427134 [details]
Patch

[Kimmo Kinnunen]

> MediaTime MediaTrackReader::greatestPresentationTime() const

Eric, Jer: 
This modifies MediaTrackReader as an example.
Can you review the fix in the greatestPresentationTime() (unsynchronized access to locked variable --> takes the lock)


Chris:
This modifies Connection.h/.cpp as an example.
Could you check out that the changes are fine?

[Chris Dumez]

Comment on attachment 427134 [details]
Patch

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

> Source/WebKit/Platform/IPC/Connection.cpp:524
> +        Locker locker { m_waitForMessageMutex };

The changes to IPC::Connection seem fine although it is kind of sad we have to change our patterns. I was really used to auto / holdLock().

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 427134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427134&action=review
> 
> > Source/WebKit/Platform/IPC/Connection.cpp:524
> > +        Locker locker { m_waitForMessageMutex };
> 
> The changes to IPC::Connection seem fine although it is kind of sad we have
> to change our patterns. I was really used to auto / holdLock().

I am also a big fan of tryHoldLock(), which in my opinion is much better than a second Locker parameter (whose name I can never remember).

[Kimmo Kinnunen]

> > The changes to IPC::Connection seem fine although it is kind of sad we have
> > to change our patterns. I was really used to auto / holdLock().
> 
> I am also a big fan of tryHoldLock(), which in my opinion is much better
> than a second Locker parameter (whose name I can never remember).

If these two style preferences are weighing more for the project than compiler checking for some thread safety issues I can certainly back the commits out.

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #8)
> > > The changes to IPC::Connection seem fine although it is kind of sad we have
> > > to change our patterns. I was really used to auto / holdLock().
> > 
> > I am also a big fan of tryHoldLock(), which in my opinion is much better
> > than a second Locker parameter (whose name I can never remember).
> 
> If these two style preferences are weighing more for the project than
> compiler checking for some thread safety issues I can certainly back the
> commits out.

I am not saying we should back things out. As I said, the changes look fine and I view this as a nit.

I am just saying that if there is a way to maintain our existing patterns while making code safer, I'd like the changes a lot more. Is there a reason we cannot make holdLock() / tryHoldLock() work?

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #9)
> while making code safer, I'd like the changes a lot more. Is there a reason
> we cannot make holdLock() / tryHoldLock() work?

The function holdLock, possibly an old workaround for the lack of language features (ctad), is too expressive. It requires move construction, which to my understanding is too powerful to be analyzed the way current analyzer works. Aliasing and conditionals are needed for move construction but hard to analyze statically.

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #10)
> (In reply to Chris Dumez from comment #9)
> > while making code safer, I'd like the changes a lot more. Is there a reason
> > we cannot make holdLock() / tryHoldLock() work?
> 
> The function holdLock, possibly an old workaround for the lack of language
> features (ctad), is too expressive. It requires move construction, which to
> my understanding is too powerful to be analyzed the way current analyzer
> works. Aliasing and conditionals are needed for move construction but hard
> to analyze statically.

Ok. If we can't make it work then fine :)

I don't know if we support try-locking yet but I'd prefer a TryLocker object instead of a second parameter to the Locker constructor.

[EWS]

Committed r276691 (237105@main): <https://commits.webkit.org/237105@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/77250681>

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #11)
> I don't know if we support try-locking yet but I'd prefer a TryLocker object
> instead of a second parameter to the Locker constructor.

Currently try pattern would be:

  if (!lock.tryLock())
      return;
  Locker locker { AdoptLockTag {}, lock };

I can of course modify it to:

  if (!lock.tryLock())
      return;
  Locker locker { AdoptLockTag, lock };


Or I can of course change it to:
 
  if (!lock.tryLock())
      return;
  AdoptedLocker locker { lock }; 

.. but I think it's a bit redundant (opinion).

This one would be not very good (opinion) but I can of course change it to that too:

  TryLocker locker;
  if (!locker.tryLock(lock))
      return;

Personally I'd think the best would be:

  if (!lock.tryLock())
      return;
   std::scoped_lock locker { std::adopt_lock_t {}, lock };

... on the grounds of mindshare, quality of documentation, simplicity, lack of need of personal stylistic opinions etc.

.. but second best in my opinion is what's there already (e.g. current specialisation Locker<CheckedLock> is a copy of the std::scoped_lock<>). But again, that's mostly just an opinion and in that way not very defendable.

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #14)
> (In reply to Chris Dumez from comment #11)
> > I don't know if we support try-locking yet but I'd prefer a TryLocker object
> > instead of a second parameter to the Locker constructor.
> 
> Currently try pattern would be:
> 
>   if (!lock.tryLock())
>       return;
>   Locker locker { AdoptLockTag {}, lock };
> 
> I can of course modify it to:
> 
>   if (!lock.tryLock())
>       return;
>   Locker locker { AdoptLockTag, lock };
> 
> 
> Or I can of course change it to:
>  
>   if (!lock.tryLock())
>       return;
>   AdoptedLocker locker { lock }; 
> 
> .. but I think it's a bit redundant (opinion).
> 
> This one would be not very good (opinion) but I can of course change it to
> that too:
> 
>   TryLocker locker;
>   if (!locker.tryLock(lock))
>       return;
> 
> Personally I'd think the best would be:
> 
>   if (!lock.tryLock())
>       return;
>    std::scoped_lock locker { std::adopt_lock_t {}, lock };
> 
> ... on the grounds of mindshare, quality of documentation, simplicity, lack
> of need of personal stylistic opinions etc.
> 
> .. but second best in my opinion is what's there already (e.g. current
> specialisation Locker<CheckedLock> is a copy of the std::scoped_lock<>). But
> again, that's mostly just an opinion and in that way not very defendable.

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.