Bug 114529 – HeapTimer lifetime should be less complicated

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 197880 [details]
Patch

[Darin Adler]

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?

[Mark Hahnenberg]

Created attachment 197912 [details]
Patch

[Early Warning System Bot]

Comment on attachment 197912 [details]
Patch

Attachment 197912 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/107068

[Early Warning System Bot]

Comment on attachment 197912 [details]
Patch

Attachment 197912 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-queues.appspot.com/results/96075

[Build Bot]

Comment on attachment 197912 [details]
Patch

Attachment 197912 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/97134

[Mark Hahnenberg]

Created attachment 197962 [details]
Patch

[Early Warning System Bot]

Comment on attachment 197962 [details]
Patch

Attachment 197962 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/95332

[Early Warning System Bot]

Comment on attachment 197962 [details]
Patch

Attachment 197962 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-queues.appspot.com/results/154081

[Mark Hahnenberg]

Created attachment 198131 [details]
Patch

[Oliver Hunt]

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()

[Mark Hahnenberg]

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).

[Mark Hahnenberg]

> 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.

[Oliver Hunt]

(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

[Mark Hahnenberg]

Created attachment 198180 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 198180 [details]
Patch

Clearing flags on attachment: 198180

Committed r148475: <http://trac.webkit.org/changeset/148475>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.