Right now our HeapTimer lifetime is rather complicated. HeapTimers are "owned" by the JSGlobalData, but there's an issue in that there can be races between a thread that is trying to tear down a JSGlobalData and the HeapTimer's fire function. Our current code for tearing down HeapTimers is an intricate and delicate dance which probably contains subtle bugs. We can make our lives easier by changing things around a bit. (1) We should free the API lock from being solely owned by the JSGlobalData so we don't have to worry about grabbing the lock out of invalid memory when our HeapTimer callback fires. (2) We should also make it so that we deref the JSGlobalData first, then unlock the API lock so that when we have the lock, the JSGlobalData is in one of two states: fully valid or completely destroyed, and we know exactly which one. (3) The JSLock can tell us this information by keeping a back pointer to the JSGlobalData. When the JSGlobalData's destructor is called, it clears this pointer in the JSLock. Other clients of the API lock can then check this pointer to determine whether or not the JSGlobalData is still around. (4) The CFRunLoopTimer will use the API lock as its context rather than the HeapTimer itself. The only way the HeapTimer's callback can get to the HeapTimer is through the API lock's JSGlobalData pointer. (5) The CFRunLoopTimerContext struct has two fields for retain and release callbacks for the context's info field. We'll provide these callbacks to ref() and deref() the JSLock as necessary. Thus, the timer becomes the other owner of the JSLock apart from the JSGlobalData.
Created attachment 197880 [details] Patch
Comment on attachment 197880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197880&action=review review- because I think you’ll want to make at least one of the changes I suggest > Source/JavaScriptCore/jsc.cpp:765 > - RefPtr<JSGlobalData> globalData = JSGlobalData::create(LargeHeap); > - JSLockHolder lock(globalData.get()); > + JSGlobalData* globalData = JSGlobalData::create(LargeHeap).leakRef(); > + APIEntryShim shim(globalData); I don’t understand this change. Change log does not make it clear. Per-function comments can sometimes help with this. > Source/JavaScriptCore/testRegExp.cpp:504 > - RefPtr<JSGlobalData> globalData = JSGlobalData::create(LargeHeap); > - JSLockHolder lock(globalData.get()); > + JSGlobalData* globalData = JSGlobalData::create(LargeHeap).leakRef(); > + APIEntryShim shim(globalData); I don’t understand this change. > Source/JavaScriptCore/heap/Heap.cpp:266 > + , m_activityCallback(adoptPtr(DefaultGCActivityCallback::create(this))) > + , m_sweeper(adoptPtr(IncrementalSweeper::create(this))) Instead of changing these call sites, the return types of the create functions should be changed to PassOwnPtr. We want the calls to adoptPtr to be right where the call to new is, not somewhere else. > Source/JavaScriptCore/heap/Heap.cpp:816 > void Heap::setActivityCallback(GCActivityCallback* activityCallback) > { > - m_activityCallback = activityCallback; > + m_activityCallback = adoptPtr(activityCallback); > } The argument type of setActivityCallback should be changed to PassOwnPtr. We want the calls to adoptPtr to be right where the call to new is, not somewhere else. > Source/JavaScriptCore/heap/HeapTimer.cpp:56 > + m_context.retain = &retainAPILock; > + m_context.release = &releaseAPILock; I don’t think these & operators are needed, and I normally prefer to omit them. > Source/JavaScriptCore/heap/HeapTimer.h:29 > +#include "JSLock.h" Why is this include needed? I don’t see any changes to the header below that seem to require it. > Source/JavaScriptCore/heap/HeapTimer.h:73 > + static const void* retainAPILock(const void*); > + static void releaseAPILock(const void*); Why do these need to be member functions? Can’t they just be free functions, private to the cpp file?
Created attachment 197912 [details] Patch
Comment on attachment 197912 [details] Patch Attachment 197912 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/107068
Comment on attachment 197912 [details] Patch Attachment 197912 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-queues.appspot.com/results/96075
Comment on attachment 197912 [details] Patch Attachment 197912 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/97134
Created attachment 197962 [details] Patch
Comment on attachment 197962 [details] Patch Attachment 197962 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/95332
Comment on attachment 197962 [details] Patch Attachment 197962 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-queues.appspot.com/results/154081
Created attachment 198131 [details] Patch
Comment on attachment 198131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198131&action=review > Source/JavaScriptCore/testRegExp.cpp:504 > - RefPtr<JSGlobalData> globalData = JSGlobalData::create(LargeHeap); > - JSLockHolder lock(globalData.get()); > + JSGlobalData* globalData = JSGlobalData::create(LargeHeap).leakRef(); > + APIEntryShim shim(globalData); Why can't we use a refptr here anymore? > Source/JavaScriptCore/API/APIShims.h:55 > JSGlobalData* m_globalData; Just make m_globalData a RefPtr<> now > Source/JavaScriptCore/API/APIShims.h:79 > + RefPtr<JSLock> apiLock = &m_globalData->apiLock(); > + m_globalData->deref(); > + apiLock->unlock(); Just make APIEntryShim have a JSLockHolder to hold on to apiLock, if it's still strictly necessary you can just do m_globalData.clear()
Comment on attachment 198131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198131&action=review >> Source/JavaScriptCore/testRegExp.cpp:504 >> + APIEntryShim shim(globalData); > > Why can't we use a refptr here anymore? Because we need to hold the lock when we destroy a JSGlobalData, but we destroy the JSLockHolder first (which releases the lock), then the RefPtr (which destroys the JSGlobalData).
> Because we need to hold the lock when we destroy a JSGlobalData, but we destroy the JSLockHolder first (which releases the lock), then the RefPtr (which destroys the JSGlobalData). I guess we could continue to use a RefPtr, but explicitly clear it prior to releasing the API lock.
(In reply to comment #12) > (From update of attachment 198131 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=198131&action=review > > >> Source/JavaScriptCore/testRegExp.cpp:504 > >> + APIEntryShim shim(globalData); > > > > Why can't we use a refptr here anymore? > > Because we need to hold the lock when we destroy a JSGlobalData, but we destroy the JSLockHolder first (which releases the lock), then the RefPtr (which destroys the JSGlobalData). What happens if we just make the api lock object refcounted, the heap can reference it, the timer can reference it, etc, etc. Then the lock automatically becomes the last thing to die
Created attachment 198180 [details] Patch
Comment on attachment 198180 [details] Patch Clearing flags on attachment: 198180 Committed r148475: <http://trac.webkit.org/changeset/148475>
All reviewed patches have been landed. Closing bug.