Bug 224970 - Add a Condition type that supports thread safety analysis
Summary: Add a Condition type that supports thread safety analysis
Status: RESOLVED FIXED
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: 221614
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-22 23:45 PDT by Kimmo Kinnunen
Modified: 2021-04-30 05:55 PDT (History)
14 users (show)

See Also:


Attachments
Patch (14.21 KB, patch)
2021-04-22 23:49 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (23.73 KB, patch)
2021-04-27 04:06 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-22 23:45:59 PDT
Add a Condition type that supports thread safety analysis
Compile should fail if that condition is waited without holding the corresponding lock.
Comment 1 Kimmo Kinnunen 2021-04-22 23:49:50 PDT
Created attachment 426890 [details]
Patch
Comment 2 Darin Adler 2021-04-25 13:53:58 PDT
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.
Comment 3 Darin Adler 2021-04-25 13:54:08 PDT
use
Comment 4 Kimmo Kinnunen 2021-04-27 04:06:46 PDT
Created attachment 427134 [details]
Patch
Comment 5 Kimmo Kinnunen 2021-04-27 04:10:59 PDT
> 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?
Comment 6 Chris Dumez 2021-04-27 08:00:45 PDT
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().
Comment 7 Chris Dumez 2021-04-27 09:54:04 PDT
(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).
Comment 8 Kimmo Kinnunen 2021-04-27 10:44:50 PDT
> > 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.
Comment 9 Chris Dumez 2021-04-27 10:47:36 PDT
(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?
Comment 10 Kimmo Kinnunen 2021-04-27 11:00:50 PDT
(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.
Comment 11 Chris Dumez 2021-04-27 11:04:47 PDT
(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.
Comment 12 EWS 2021-04-27 23:26:22 PDT
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].
Comment 13 Radar WebKit Bug Importer 2021-04-27 23:27:13 PDT
<rdar://problem/77250681>
Comment 14 Kimmo Kinnunen 2021-04-27 23:34:25 PDT
(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.
Comment 15 Chris Dumez 2021-04-28 07:55:58 PDT
(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.