Bug 94761 – ThreadRestrictionVerifier should be opt-in, not opt-out

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

ThreadRestrictionVerifier should be opt-in, not opt-out

[Geoffrey Garen]

Created attachment 160047 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 160047 [details]
Patch

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

r=me!

> Source/ThirdParty/ANGLE/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Where did this ChangeLog come from?

[David Levin]

Comment on attachment 160047 [details]
Patch

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

This change appears to make it easier for JSC code to turn this off which is good.

How does the rest of the code continue to get the benefit from this automatically as it has in the past? (There have been several issues uncovered by this which would have resulted in hard to figure out memory corruptions.)

> Source/WTF/ChangeLog:23
> +        to maintain with default off, so I removed them.

Actually they aren't. They simply assert that the method calls were only made when the class was in its initial state.  It is only the change to make m_mode NoVerificationMode that makes them fire.

[David Levin]

It sounds like the concern is that jsc is the primary place where threading is done so this is the place bitten by this assert most often.

Honestly, I'm not very connected lately with WebKit so if folks actively working on the project feel this is best. Please go ahead!

Thanks for taking the time to response to me in irc.

[Mark Hahnenberg]

Comment on attachment 160047 [details]
Patch

It looks like David is ok with this, so r=me again!

[Geoffrey Garen]

Committed r126379: <http://trac.webkit.org/changeset/126379>

[Alexey Proskuryakov]

I strongly object to this change. ThreadRestrictionVerifier has been finding bugs in places we would never think of oping in for verification. JavaScriptCore is certainly not the only part of WebKit that does threading.

[Geoffrey Garen]

> I strongly object to this change.

OK, I hear your objection.

Do you have a proposal for how to make our tests stop ASSERTing for any client that uses threads?

[Alexey Proskuryakov]

Do we know when ASSERTing started, and what specific parts of JSC cause it?

DumpRenderTree runs code at startup that uses threads, so one can definitely use JSC API from a thread - otherwise, no regression tests would run.

[Geoffrey Garen]

> Do we know when ASSERTing started, and what specific parts of JSC cause it?

This mechanism has caused ASSERTs to fire in JSC since its first introduction. It was designed with the assumption of a single-threaded VM (v8).

The ASSERTs fire for anything RefPtr in Source/WTF or Source/JavaScriptCore.

> DumpRenderTree runs code at startup that uses threads, so one can definitely use JSC API from a thread - otherwise, no regression tests would run.

DumpRenderTree doesn't use the feature of using an object on multiple threads.

[David Levin]

(In reply to comment #10)
> > Do we know when ASSERTing started, and what specific parts of JSC cause it?
> 
> This mechanism has caused ASSERTs to fire in JSC since its first introduction. It was designed with the assumption of a single-threaded VM (v8).

fwiw, I didn't design it for single-thread vm. v8 doesn't even use wtf as far as I know.

I did it for WebCore (workers, icon db, index db, which use multiple threads, etc), etc. I tried to quiet JSC things that I hit. (I had intended to add something JSC specific that could check for locks rather than just turning it off but it still would need to go in each ref counted class in JSC -- I just never got around to it before I mostly disappeared).


> 
> The ASSERTs fire for anything RefPtr in Source/WTF or Source/JavaScriptCore.
> 
> > DumpRenderTree runs code at startup that uses threads, so one can definitely use JSC API from a thread - otherwise, no regression tests would run.
> 
> DumpRenderTree doesn't use the feature of using an object on multiple threads.

[Geoffrey Garen]

> > This mechanism has caused ASSERTs to fire in JSC since its first introduction. It was designed with the assumption of a single-threaded VM (v8).
> 
> fwiw, I didn't design it for single-thread vm. v8 doesn't even use wtf as far as I know.

OK, it was designed with the assumption that WebCore is the only client of WTF (which is only true in Chromium).

[David Levin]

(In reply to comment #12)
> > > This mechanism has caused ASSERTs to fire in JSC since its first introduction. It was designed with the assumption of a single-threaded VM (v8).
> > 
> > fwiw, I didn't design it for single-thread vm. v8 doesn't even use wtf as far as I know.
> 
> OK, it was designed with the assumption that WebCore is the only client of WTF (which is only true in Chromium).

I appreciate that you're trying to be generous, and I'm sorry that it didn't accomodate JSC better.

Rather than wax poetically about my intentions, here's the data I see:

JavaScriptCore, 12 classes which use RefCounted as a base class.

http://code.google.com/p/chromium/source/search?q=c%5C+RefCounted%5C%3C+file%3Athird_party%2FWebKit%2FSource%2FJavaScriptCore&origq=c%5C+RefCounted%5C%3C+file%3Athird_party%2FWebKit%2FSource%2FJavaScriptCore&btnG=Search+Trunk

WTF, 5 classes which use RefCounted as a base classes.

http://code.google.com/p/chromium/source/search?q=c%5C+RefCounted%5C%3C+file%3Athird_party%2FWebKit%2FSource%2FWTF&origq=c%5C+RefCounted%5C%3C+file%3Athird_party%2FWebKit%2FSource%2FWTF&btnG=Search+Trunk

Other directories, ~500 classes which use RefCounted as a base class.

http://code.google.com/p/chromium/source/search?q=c%5C+RefCounted%5C%3C+file%3Athird_party%2FWebKit&origq=c%5C+RefCounted%5C%3C+file%3Athird_party%2FWebKit&btnG=Search+Trunk

The default was set for those 500 classes in WebKit (not the other 17). The thread checker class had escape hatches for the special cases (which I put in places as I encountered them and I tried JSC in DRT and my debug WebKit build in Safari -- though any of my WebKit builds always hung due to some plugin thing in the Safari side of the code).

As to this patch, folks working on WebKit should decide what is the best code to run in WebKit (that isn't me at present).

[Geoffrey Garen]

> JavaScriptCore, 12 classes which use RefCounted as a base class.
> WTF, 5 classes which use RefCounted as a base classes.

OK, does anybody have a proposal for what to do with these 17 classes?

[David Levin]

(In reply to comment #14)
> > JavaScriptCore, 12 classes which use RefCounted as a base class.
> > WTF, 5 classes which use RefCounted as a base classes.
> 
> OK, does anybody have a proposal for what to do with these 17 classes?

Call "turnOffVerifier();" in their constructors? (Or just leave in this patch.)

I didn't do this by default in the past because my approach with this was only to add that when I hit an assert (or someone else hit one, etc.) because I felt like this was the cautious path (verifying that each class I added it to was indeed an exception).  However, I now realize that was over cautious for these classes.

Ideally this wouldn't be in the 5 wtf classes unless they were used by JSC. However, if they are used in JSC,  turning off the checking for them seems like a fine compromise.

[Geoffrey Garen]

> Call "turnOffVerifier();" in their constructors?

I would probably use a super-type of RefCounted (UnverifiedRefCounted? FreedomRefCounted?), to make the find-and-replace simpler, and leave a clearer message to future coders about the design rule.

But...

> Ideally this wouldn't be in the 5 wtf classes unless they were used by JSC.

JSC uses fundamental WTF types, like StringImpl. So, a solution to turn things on by default in WebCore will still leave the verifier off for many of the classes in which it has caught bugs before.

My understanding is that those WTF classes gave us most of the value before, which is why I didn't see much difference between a targeted solution and just turning it all off: either way, to get the value out of the verifier, you have to pick specific strings, etc., in WebCore and opt them into verification.

Alexey, David, do you like the solution of "off by default in WTF and JSC types, on by default in WebCore types"?

[David Levin]

(In reply to comment #16)
> > Call "turnOffVerifier();" in their constructors?
> 
> I would probably use a super-type of RefCounted (UnverifiedRefCounted? FreedomRefCounted?), to make the find-and-replace simpler, and leave a clearer message to future coders about the design rule.
> 
> But...
> 
> > Ideally this wouldn't be in the 5 wtf classes unless they were used by JSC.
> 
> JSC uses fundamental WTF types, like StringImpl. So, a solution to turn things on by default in WebCore will still leave the verifier off for many of the classes in which it has caught bugs before.
> 
> My understanding is that those WTF classes gave us most of the value before, which is why I didn't see much difference between a targeted solution and just turning it all off: either way, to get the value out of the verifier, you have to pick specific strings, etc., in WebCore and opt them into verification.
> 
> Alexey, David, do you like the solution of "off by default in WTF and JSC types, on by default in WebCore types"?

fwiw, imo, that is a fine compromise. I don't remember any instances in which those found issues anyway.

More context:
The WebCore types (and higher) are actually the places where I have seen usefulness.

fwiw, StringImpl doesn't use RefCounted so unfortunately strings never get this check. There are likely issue with strings. Do I need to say that it was on mental list to get to it (but then I did a DB Cooper -- w/o the extortion and ransom)? :)

[Alexey Proskuryakov]

> Alexey, David, do you like the solution of "off by default in WTF and JSC types, on by default in WebCore types"?

Makes sense to me, too.