Bug 95244 – ExecutableAllocator should be destructed after Heap

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yong Li]

It is said 

    public:
        Heap heap; // The heap is our first data member to ensure that it's destructed after all the objects that reference it.

But actually Heap can access MetaAllocator

#6  WTF::MetaAllocatorHandle::~MetaAllocatorHandle
#10 ~MacroAssemblerCodeRef
#11 JSC::JITStubRoutine::~JITStubRoutine
#13 JSC::GCAwareJITStubRoutine::~GCAwareJITStubRoutine
#16 JSC::Heap::~Heap
#17 JSC::JSGlobalData::~JSGlobalData

This is only a problem when ENABLE(ASSEMBLER_WX_EXCLUSIVE) is true, where every ExecutableAllocator has a MetaAllocator instead of sharing the global one.

But I think it is always good to make ExecutableAllocator construct before Heap and destruct after Heap as it never calls Heap, but Heap depends on ExecutableAllocator.

[Yong Li]

Created attachment 161222 [details]
the patch

[Early Warning System Bot]

Comment on attachment 161222 [details]
the patch

Attachment 161222 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13681246

[Early Warning System Bot]

Comment on attachment 161222 [details]
the patch

Attachment 161222 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13695228

[Yong Li]

Created attachment 161238 [details]
fix the build error

[Geoffrey Garen]

Comment on attachment 161238 [details]
fix the build error

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

Did you check to make sure the Executable Allocator doesn't use the heap in its constructor?

> Source/JavaScriptCore/runtime/JSGlobalData.cpp:124
> +    , heap(this, heapType)

No need for this initializer inside the #if.

[Yong Li]

(In reply to comment #5)
> (From update of attachment 161238 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161238&action=review
> 
> Did you check to make sure the Executable Allocator doesn't use the heap in its constructor?

Yes. ExecutableAllocator knows nothing about the heap. It doesn't even not use the JSGlobalData& argument. (checked in both ExecutableAllocator.cpp and ExecutableAllocatorFixedVMPool.cpp. It is just used to allocate memory for JIT assemblies. So logically it shouldn't rely on any other JS data.

> 
> > Source/JavaScriptCore/runtime/JSGlobalData.cpp:124
> > +    , heap(this, heapType)
> 
> No need for this initializer inside the #if.

Just for "," and ":"

Would this be OK then?

JSGlobalData::JSGlobalData(GlobalDataType globalDataType, ThreadStackType threadStackType, HeapType heapType)
    : 
#if ENABLE(ASSEMBLER)
      executableAllocator(*this),
#endif
      heap(this, heapType)
    , globalDataType(globalDataType)

[Geoffrey Garen]

> Would this be OK then?

Yes.

[Yong Li]

Created attachment 161266 [details]
Again

[Rob Buis]

Comment on attachment 161266 [details]
Again

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

Looks good.

> Source/JavaScriptCore/ChangeLog:9
> +        Existing Web Worker tests can show the issue.

Can you include PR?

[Yong Li]

Created attachment 161286 [details]
To commit

[Yong Li]

Created attachment 161287 [details]
to commit

[Yong Li]

Landed @r127034.