[webkit-reviews] review denied: [Bug 31639] Add asserts to RefCounted to make sure ref/deref happens on the right thread. : [Attachment 88739] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied David Levin <levin at chromium.org>'s
request for review:
Bug 31639: Add asserts to RefCounted to make sure ref/deref happens on the
right thread.
https://bugs.webkit.org/show_bug.cgi?id=31639

Attachment 88739: Patch
https://bugs.webkit.org/attachment.cgi?id=88739&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88739&action=review

Looks good. Not sure why it’s not applying with the EWS. I’m going to say
review- because I want you to think over the naming a little more.

> Source/JavaScriptCore/jit/ExecutableAllocator.h:353
> +    reviewCarefullyTurnOffVerifier();

I applaud the notion of building the advice to review the use of a function
carefully right into the function name, but I think we should do it in a way
that’s more grammatically sound. One type of name along those lines is this:

turnOffVerifierNotingThatIsRarelyNeededAndRequiresCarefulReview

Of course that is comically long, but maybe you see my point? Once we figure
out what we should say, we should try to state it rather than just putting
words in there without grammatical glue.

> Source/JavaScriptCore/wtf/NonThreadSafeVerifier.h:46
> +class NonThreadSafeVerifier {

I think that “non-safe” is an unpleasant and not entirely clear way to describe
something that is designed to be used on a single thread.

I would call this ThreadRestrictionsVerifier or SingleThreadUsageVerifier or
something roughly along those lines. What I don’t like about “not safe” is that
clearly you can’t build a good computer program out of pieces that are
“unsafe”. The use of “safety” as the metaphor for threads programming is one of
the things that makes it harder for people. Instead of placing the emphasis on
clear rules, there’s the implication that this is about risk taking, things
might work if you just look both ways before crossing the street, and bold
adventurous programmers might chose to take risks. That’s why the safety
metaphor seems the wrong one to me.

> Source/JavaScriptCore/wtf/RefCounted.h:44
> +	   // Start thread verification as soon as the ref count gets to 2.
This
> +	   // is a heuristic but it seems to work and has helped find some
bugs.

It would be better to explain where this heuristic comes from. There must be a
relatively clear way to explain it. Maybe what you mean is that it’s common to
create an object and then hand it over to another thread; always recording the
thread where it was created would lead to too many false failures.

This is an OK policy for existing code, but it seems inelegant for new code, so
we might want to consider a way that future uses of RefCount could start
verifying immediately. I would like to catch an object created on one thread
and destroyed on another without ever being ref'd.

Again, we can probably defend this policy, and I think it’s better to do that
with a little more specifics even if not many more words.

> Source/JavaScriptCore/wtf/SizeLimits.cpp:45
> +struct FakeDebugRefCounted {

I don’t entirely understand the meaning of the word “Fake” in this structure’s
name. You should choose an even clearer name or add a comment.