[webkit-reviews] review granted: [Bug 200507] Add threading assertions to RefCounted : [Attachment 376043] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 200507: Add threading assertions to RefCounted
https://bugs.webkit.org/show_bug.cgi?id=200507

Attachment 376043: Patch

https://bugs.webkit.org/attachment.cgi?id=376043&action=review




--- Comment #47 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 376043
  --> https://bugs.webkit.org/attachment.cgi?id=376043
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +	   * dfg/DFGPlan.cpp:
> +	   (JSC::DFG::Plan::Plan):

Could you explain why we want to disable the assertion here?

> Source/WTF/ChangeLog:9
> +	   Add threading assertions to RefCounted to try and catch non-thread
safe using of
> +	   RefCounted objects. These assertions also found several thread
safety bugs in our

Instead of saying non-thread safe use, can we just say that cross-thread /
concurrent use of RefCounted objects
and note that ThreadSafeRefCounted should be used instead?
As in, explain why using RefCounted in different threads is unsafe so that
people reading this commit message 
can easily figure out what needs to be done when they encounter this assertion
failure.

> Source/WTF/wtf/RefCounted.h:46
> +	   ASSERT(!areThreadingCheckedEnabled() || m_isOwnedByMainThread ==
isMainThread());

Please add a comment saying that we should be using ThreadSafeRefCounted
instead
if objects need to be shared / used across threads.

> Source/WTF/wtf/RefCounted.h:106
> +    bool areThreadingCheckedEnabled() const

Could you add a comment clarifying when this function can be used?
I'm afraid people would start calling this in a bunch of other places when they
hit this assertion.