Bug 31639

Created attachment 43545 [details] Intermediate patch I've tried to experiment with something like this... Intermediate patch. There is makeExemptFromThreadCheck() function that stores a per-instance flag which I was hoping to use to have some sort of 'dynamic opt-out'. To make it possible to 'temporarily exclude' an instance of RefCounted from the requirement to be refcounted on the same thread - and then use it for things like String, relaxing the check in certain portions of code and still asserting in others. But IconDatabase quickly discouraged me from this... It is very hard, looking at code, to know where to enable/disable the check. Usage on different threads can happen in way distant codepaths.

Created attachment 44201 [details] Proposed patch. Adds ASSERT to RefCountedBase to verify the ref()/deref() and hasOneref(), refCount() happen on the same thread. For objects that are intentionally used in cross-process scenario, there is disableThreadVerification() which excludes them from the checks. I used this to exclude Stringimpl and classes used in IconDatabase. Also, the checks only are activated when refcount becomes >= 2 and deactivated when refcount goes below 2 (borrowed the idea from CrossThreadRefCounted). This prevents the assertions in a very typical scenario when object is created on one thread and then passed via PassRefPtr to another thread where it is used exclusively and then destroyed. It seems it's mostly ok to transfer an object with refcount 1 between threads - so the asserts are only activated when refcount is >1, meaning the object is actually 'shared'. Perf impact on Debug build is about 2% on run-webkit-tests, and there are no asserts fired during the run.

style-queue ran check-webkit-style on attachment 44201 [details] without any errors.

Comment on attachment 44201 [details] Proposed patch. > diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog > + (WTF::ThreadVerifier::activate): Activates hecks. Called when ref count becomes above 2. typo: hecks > diff --git a/JavaScriptCore/runtime/Structure.cpp b/JavaScriptCore/runtime/Structure.cpp > + // Use of addressOfCount() in this calss prevents thread verification in RefCounted typo: calss > + // since it modifies refcount via other means then ref()/deref(). s/then/than/ > diff --git a/JavaScriptCore/wtf/RefCounted.h b/JavaScriptCore/wtf/RefCounted.h > +#ifndef NDEBUG > +class ThreadVerifier { This seems like it should be in its own file. (As a side benefit, the whole file can be ifndef NDEBUG include the wtf/Threading.h include.) > +#endif #endif // NDEBUG Since this is a long ifdef. > +#ifndef NDEBUG > + void disableThreadVerification() > + { Personally, I'd prefer moving the ifndef inside of the method. This would still make the call a no-op when NDEBUG is defined, but every call site wouldn't need a ifndef NDEBUG around it. > + m_threadVerifier.disableThreadVerification(); > + } > +#endif > +

Created attachment 44423 [details] Updated per David's comments.

(In reply to comment #4) > typo: hecks > typo: calss > s/then/than/ Fixed. > > diff --git a/JavaScriptCore/wtf/RefCounted.h b/JavaScriptCore/wtf/RefCounted.h > > +#ifndef NDEBUG > > +class ThreadVerifier { > > This seems like it should be in its own file. (As a side benefit, the whole > file can be ifndef NDEBUG include the wtf/Threading.h include.) Moved to a new file, wtf/ThreadVerifier.h This added a couple of build files and ForwardingHeaders to the patch. > #endif // NDEBUG > Since this is a long ifdef. Added "// NDEBUG" > > +#ifndef NDEBUG > > + void disableThreadVerification() > > + { > > Personally, I'd prefer moving the ifndef inside of the method. This would still > make the call a no-op when NDEBUG is defined, but every call site wouldn't need > a ifndef NDEBUG around it. Done. Patch attached.

style-queue ran check-webkit-style on attachment 44423 [details] without any errors.

Comment on attachment 44423 [details] Updated per David's comments. The only thing I don't like about this is the name of disableThreadVerification. I would prefer a name that describes the threading regime being used rather than a simple "disable" call. But since the number of callers is extremely small at this time, it seems we can change this later. I'd eventually like to see the cross-thread string use be turned on, not in the allocation of the StringImpl, but at the call sites. I know the class is used cross thread "everywhere", but I think eventually we could do that. There are tons of strings where it would be good to have the main thread assertion work. > + // Start thread verification as soon as the ref count gets to 2. > + // The class gets created with a ref count of 1 and then passed > + // to another thread where to ref count get increased. This > + // is a heuristic but it seems to always work and has helped > + // find some bugs. We use one space after a period, nottwo. > + ThreadVerifier() > + : m_isActive(false) > + , m_isThreadVerificationDisabled(false) > + , m_isOwnedByMainThread(false) > + , m_owningThread(0) > + { } We'd normally put the braces on separate lines. > @@ -44,7 +44,7 @@ IconRecord::IconRecord(const String& url) > , m_stamp(0) > , m_dataSet(false) > { > - > + disableThreadVerification(); > } I'd like to see a comment at all the disableThreadVerification call sites about why this needs to be done.

(In reply to comment #8) > (From update of attachment 44423 [details]) > The only thing I don't like about this is the name of > disableThreadVerification. I would prefer a name that describes the threading > regime being used rather than a simple "disable" call. But since the number of > callers is extremely small at this time, it seems we can change this later. Agreed. > I'd eventually like to see the cross-thread string use be turned on, not in the > allocation of the StringImpl, but at the call sites. I know the class is used > cross thread "everywhere", but I think eventually we could do that. There are > tons of strings where it would be good to have the main thread assertion work. Added FIXME to StringImpl, will experiment with enforcing checks since we should be able to use crossThreadString in those cases. .. > We use one space after a period, nottwo. .. > We'd normally put the braces on separate lines. .. > I'd like to see a comment at all the disableThreadVerification call sites about > why this needs to be done. Fixed those and landed: http://trac.webkit.org/changeset/51869

Reopening since I've reverted it: http://trac.webkit.org/changeset/51875 It apparently caused stable crash of storage/change-version-handle-reuse.html on Leopard Debug bot. Also, the nmber of reported leaks increased about x10 on SnowLeopard leak-test-enabled bot. Need to play with it a bit more.

(In reply to comment #10) > Also, the number of reported leaks increased about x10 on SnowLeopard leak-test-enabled bot. Wow, I wonder what that was about? I really want this back!

Comment on attachment 44423 [details] Updated per David's comments. Clearing Darin Adler's review on this obsolete patch.

Created attachment 88125 [details] Patch

Attachment 88125 [details] did not build on win: Build output: http://queues.webkit.org/results/8330130

Comment on attachment 88125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88125&action=review review- because the Windows build is failing > Source/JavaScriptCore/JavaScriptCore.exp:608 > +__ZN3WTF26singleThreadVerifierCreateEv > +__ZN3WTF20noVerificationCreateEv > +__ZN3WTF19mutexVerifierCreateERNS_5MutexE This file is sorted with the sort tool. New entries in it should be sorted in the same way. > Source/JavaScriptCore/jit/ExecutableAllocator.h:354 > + setNonThreadSafeVerifier(WTF::NonThreadSafeVerifier::createNoVerficationDoNotUseThis()); Classes in WTF should make use of "using" in the WTF way and not have WTF:: at the call site. > Source/JavaScriptCore/parser/SourceProvider.h:50 > + setNonThreadSafeVerifier(WTF::NonThreadSafeVerifier::createNoVerficationDoNotUseThis()); While factoring-wise I think it’s OK to have the verifier be a separate object, I think that it involves tons of memory allocation in debug builds and so could slow them to a crawly, and the idiom of setting up a verifier is so wordy that it almost obscures the meaning of what’s being done. > Source/JavaScriptCore/wtf/NonThreadSafeVerifier.cpp:52 > + virtual void setShared(bool); > + virtual bool verifySafety() const; I suggest making these private instead of public on the derived class. > Source/JavaScriptCore/wtf/NonThreadSafeVerifier.cpp:76 > + virtual void setShared(bool); > + virtual bool verifySafety() const; Ditto. > Source/JavaScriptCore/wtf/NonThreadSafeVerifier.cpp:183 > +PassOwnPtr<NonThreadSafeVerifier> singleThreadVerifierCreate() > +{ > + return 0; > +} > + > +PassOwnPtr<NonThreadSafeVerifier> mutexVerifierCreate(Mutex&) > +{ > + return 0; > +} > + > +PassOwnPtr<NonThreadSafeVerifier> noVerificationCreate() > +{ > + return 0; > +} Why define these at all in NDEBUG versions? > Source/JavaScriptCore/wtf/NonThreadSafeVerifier.h:54 > + static PassOwnPtr<NonThreadSafeVerifier> createNoVerficationDoNotUseThis(); Typo: Verfication > Source/JavaScriptCore/wtf/NonThreadSafeVerifier.h:68 > +// Don't use these directly. > +PassOwnPtr<NonThreadSafeVerifier> singleThreadVerifierCreate(); > +PassOwnPtr<NonThreadSafeVerifier> mutexVerifierCreate(Mutex&); > +PassOwnPtr<NonThreadSafeVerifier> noVerificationCreate(); Why not? Why do we need the extra level of indirection? If we don’t want them used directly, then we could obfuscate them a little more by putting them into a namespace or making them private static member functions. And we could not even define them if NDEBUG is set. > Source/JavaScriptCore/wtf/RefCounted.h:45 > + // to another thread where to ref count get increased. This “where to ref count get increased”? > Source/JavaScriptCore/wtf/RefCounted.h:88 > + void setNonThreadSafeVerifier(PassOwnPtr<NonThreadSafeVerifier> nonThreadSafeVerifier) > + { > + UNUSED_PARAM(nonThreadSafeVerifier.get()); > +#ifndef NDEBUG > + m_nonThreadSafeVerifier = nonThreadSafeVerifier; > +#endif > + } > + > + void setMutexAsThreadSafeVerifier(Mutex& mutex) > + { > + UNUSED_PARAM(&mutex); > +#ifndef NDEBUG > + setNonThreadSafeVerifier(NonThreadSafeVerifier::createMutexVerifier(mutex)); > +#endif > + } I suggest putting the whole functions inside an #ifndef to avoid the awkward uses of UNUSED_PARAM. I also question the need to inline the debug versions. I think you could just declare these in the class, define the NDEBUG versions below with inline, and for the debug versions have them in a cpp file. > Source/JavaScriptCore/wtf/RefCounted.h:165 > + OwnPtr<NonThreadSafeVerifier> m_nonThreadSafeVerifier; I think this data member could just be named m_verifier without losing clarity.

Comment on attachment 88125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88125&action=review >> Source/JavaScriptCore/wtf/NonThreadSafeVerifier.cpp:183 >> +} > > Why define these at all in NDEBUG versions? I couldn't figure out a way to have DEBUG only exports from JavaScriptCore.exp or if there was a separate file.

(In reply to comment #16) > > Why define these at all in NDEBUG versions? > > I couldn't figure out a way to have DEBUG only exports from JavaScriptCore.exp or if there was a separate file. Good point. I’m not sure there is a good way to do that.

Created attachment 88725 [details] Patch

I fixed most things. I did a few things due to the exports issue. Also, I left functions defined in the header even in release mode but empty so that the calling sites would not have to have ifndef NDEBUG whereever these methods were called. But I'm willing to change any of this. I tried to check out the speed of this build compared to without these checks and it seemed comparable for running layout tests.

Attachment 88725 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8347778

(In reply to comment #20) > Attachment 88725 [details] did not build on chromium: > Build output: http://queues.webkit.org/results/8347778 I will put the code NonThreadSafeVerifier.h inside of "#ifndef NDEBUG"

Created attachment 88739 [details] Patch

Comment on attachment 88739 [details] Patch 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.

Thanks very much for tackling this!

(In reply to comment #24) > Thanks very much for tackling this! Thanks for carefully reviewing it! :)

Created attachment 88896 [details] Patch

(In reply to comment #23) > (From update of attachment 88739 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88739&action=review > >Not sure why it’s not applying with the EWS. I had not sync'ed in a bit. Now I have. > > > 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, I made what the method does totally opaque now from the calling site but made it clear that no one should call it -- "neverCallThis". It is my goal to get rid of the method all together (and my current hope is to not get more instances of it in the code base). > > Source/JavaScriptCore/wtf/NonThreadSafeVerifier.h:46 > > +class NonThreadSafeVerifier { > I would call this ThreadRestrictionsVerifier Done. > > > 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. > > I would like to catch an object created on one thread and destroyed on another without ever being ref'd. Added a bug for being more strict. I think we can get there but it will take a bit of time. > > 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. Attempted to explain it better. > > 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. Attempted to clarify the wording.

Created attachment 88910 [details] Patch

Comment on attachment 88910 [details] Patch From ggaren, "deprecatedXXX" is usually how we migrate off of old patterns, as opposed to "neverCallThis" so, "deprecatedTurnOffThreadingVerification", or something like that)

Created attachment 90933 [details] Patch

Created attachment 91371 [details] Patch

Comment on attachment 91371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91371&action=review > Source/WebCore/ChangeLog:5 > + Extra line? > Source/WebCore/ForwardingHeaders/wtf/ThreadRestrictionVerifier.h:2 > +#define WebCore_FWD_ThreadRestrictionVerifier_h Is this the standard? I don't know.

Created attachment 91374 [details] Patch

(In reply to comment #32) > (From update of attachment 91371 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91371&action=review > > > Source/WebCore/ChangeLog:5 > > + > > Extra line? Fixed. > > > Source/WebCore/ForwardingHeaders/wtf/ThreadRestrictionVerifier.h:2 > > +#define WebCore_FWD_ThreadRestrictionVerifier_h > > Is this the standard? Yep (I checked several files at random in that dir and they all followed this pattern).

Comment on attachment 91374 [details] Patch I should probably land this by hand when it gets r+. Although I've tried hard to make sure there won't be issues, this is a patch that has a higher probability of causing issues.

Comment on attachment 91374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91374&action=review > Source/JavaScriptCore/wtf/RefCounted.h:77 > + UNUSED_PARAM(&mutex); > +#ifndef NDEBUG > + m_verifier.setMutexMode(mutex); > +#endif We should only use UNUSED_PARAM when it’s actually unused. So it should be in an #else. But, better, I suggest writing it like this: void setMutexForVerifier(Mutex&); ... #ifdef NDEBUG inline void RefCountedBase::setMutexForVerifier(Mutex&) { } #else inline void RefCountedBase::setMutexForVerifier(Mutex& mutex) { m_verifier.setMutexMode(mutex); } #endif Or like this: #ifdef NDEBUG void setMutexForVerifier(Mutex&) { } #else void setMutexForVerifier(Mutex& mutex) { m_verifier.setMutexMode(mutex); } #endif > Source/JavaScriptCore/wtf/ThreadRestrictionVerifier.h:95 > + // Is it ok to use the object at this moment on the current thread? Normally "ok" is spelled "OK". > Source/JavaScriptCore/wtf/ThreadRestrictionVerifier.h:104 > + // isMainThread() is way faster than currentThread() on many platforms, so use it first. > + return (m_isOwnedByMainThread && isMainThread()) || (m_owningThread == currentThread()); If m_isOwnedByMainThread is false, then m_owningThread is not something we should look at; sure, it’s initialized to 0, but I’m not sure that’s even meaningful. So I suggest writing: return m_isOwnedByMainThread ? isMainThread() : (m_owningThread == currentThread()); Or factoring the code some other way, but doing that same effective check. I don’t think it’s good to look at m_owningThread at all if it wasn’t set to a thread.

Committed as http://trac.webkit.org/changeset/85233. (In reply to comment #36) > (From update of attachment 91374 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91374&action=review > > > Source/JavaScriptCore/wtf/RefCounted.h:77 > But, better, I suggest writing it like this: Thanks for the detailed explanation. You said this before, and it is trivial to understand what you meant now that you wrote it out, but I just didn't get it previously. > > Source/JavaScriptCore/wtf/ThreadRestrictionVerifier.h:104 > I don’t think it’s good to look at m_owningThread at all if it wasn’t set to a thread. Thanks. That shouldn't have been written that way and was essentially a bug. Fixed (as you suggested).

http://trac.webkit.org/changeset/85233 might have broken Qt Linux ARMv7 Release and EFL Linux Release (Build)

Fixed build -- inverted ifdef (that I reversed during my last round of fixes) here: http://trac.webkit.org/changeset/85235 (I did another debug build but not another release build which would have caught this.)

Had to rollout due to jsc issue.

Comment on attachment 91374 [details] Patch This broke the jsc api/jsc command line app because they don't have a sense of what is the main thread and may never set the main thread. I need to add some code to handle this case.

Created attachment 101538 [details] Patch

Same patch as what I checked in (which is the last patch with the suggested changes) plus the following changes: 1. This no longer relies on isMainThread since currentThread is now nearly as fast in debug. 2. I fixed the inverted ifndef that I changed here: http://trac.webkit.org/changeset/85235

Created attachment 102601 [details] Patch

Removed the exception for CString since the Chromium test code causing a failure has been fixed.

Comment on attachment 102601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102601&action=review r=me, one tiny nit. It's so cool you are doing this! The amount of things already fixed in order to land this patch is already worth the trouble :-) > Source/JavaScriptCore/wtf/ThreadRestrictionVerifier.h:45 > +// The mode may be change by calling useMutexMode (or turnOffVerification). "may be change" -> "may be changed"

Comment on attachment 102601 [details] Patch Marking cq- as it looks like this triggers asserts in Chromium. I suspect this are due to access of WebKit before calling WebKit::initialize.

Committed as http://trac.webkit.org/changeset/92254

(In reply to comment #48) > Committed as http://trac.webkit.org/changeset/92254 If a SharedBuffer is set with MutexVerificationMode, then how can user own/lock it for further operation? For example, while user gets a image via : WebCore::Image *icon = WebCore::iconDatabase().synchronousIconForPageURL(kurl.string(), WebCore::IntSize(16, 16)); How to make sure calling "icon->nativeImageForCurrentFrame()" is safe?

(In reply to comment #49) > (In reply to comment #48) > > Committed as http://trac.webkit.org/changeset/92254 > > > If a SharedBuffer is set with MutexVerificationMode, then how can user own/lock it for further operation? > > For example, while user gets a image via : > > WebCore::Image *icon = WebCore::iconDatabase().synchronousIconForPageURL(kurl.string(), WebCore::IntSize(16, 16)); > > > How to make sure calling "icon->nativeImageForCurrentFrame()" is safe? If an object is using MutexVerificationMode, it means that a mutex must be taken (locked) when the object is being used. The specific mutex depends on the object but you should be able to determine if by looking at the place where the mutex verification mode is set up.

> > If an object is using MutexVerificationMode, it means that a mutex must be taken (locked) when the object is being used. The specific mutex depends on the object but you should be able to determine if by looking at the place where the mutex verification mode is set up. Yes, thanks. However I am calling 'ewk_history_item_icon_surface_get()' recently(in Source/WebKit/efl/ewk/ewk_history.cpp), it always gets exception like this: ASSERTION FAILED: m_verifier.isSafeToUse() ../Source/JavaScriptCore/wtf/RefCounted.h(53) : void WTF::RefCountedBase::ref() Below is the code, it gets exception at "icon->nativeImageForCurrentFrame()". And have no idea how to take/lock the data before further operation? ----------------------------------------------------------------------- However, cairo_surface_t* ewk_history_item_icon_surface_get(const Ewk_History_Item* item) { EWK_HISTORY_ITEM_CORE_GET_OR_RETURN(item, core, 0); WebCore::Image* icon = WebCore::iconDatabase().synchronousIconForPageURL(core->url(), WebCore::IntSize(16, 16)); if (!icon) { ERR("icon is NULL."); return 0; } return icon->nativeImageForCurrentFrame(); }

(In reply to comment #51) > However I am calling 'ewk_history_item_icon_surface_get()' recently(in Source/WebKit/efl/ewk/ewk_history.cpp), it always gets exception like this: > > ASSERTION FAILED: m_verifier.isSafeToUse() > ../Source/JavaScriptCore/wtf/RefCounted.h(53) : void WTF::RefCountedBase::ref() > > > Below is the code, it gets exception at "icon->nativeImageForCurrentFrame()". And have no idea how to take/lock the data before further operation? We’ll need the whole backtrace to understand what the issue is. I suggest opening a new bug report to track this.

(In reply to comment #52) > > We’ll need the whole backtrace to understand what the issue is. I suggest opening a new bug report to track this. New bug opened at https://bugs.webkit.org/show_bug.cgi?id=67582