[bmalloc] IsoTLSLayout and AllIsoHeaps registration is racy with derived class initialization with virtual functions
<rdar://problem/55007731>
Yeah! I reproduced this race with unit-test!! 0 **PASS** bmalloc.IsoHeapMultipleThreadsWhileIterating 1 **PASS** bmalloc.IsoHeapMultipleThreadsWhileIterating 2 **PASS** bmalloc.IsoHeapMultipleThreadsWhileIterating 3 **PASS** bmalloc.IsoHeapMultipleThreadsWhileIterating 4 **PASS** bmalloc.IsoHeapMultipleThreadsWhileIterating 5 **PASS** bmalloc.IsoHeapMultipleThreadsWhileIterating 6 **PASS** bmalloc.IsoHeapMultipleThreadsWhileIterating 7 **PASS** bmalloc.IsoHeapMultipleThreadsWhileIterating 8 libc++abi.dylib: Pure virtual function called!
Created attachment 377960 [details] Patch
I'll quickly fix GTK EWS failure.
Created attachment 377962 [details] Patch
Created attachment 377963 [details] Patch
Comment on attachment 377963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377963&action=review Nice catch. This is an interesting by-product of the C++ spec requirement that the instance vtbl pointer must point to the base class vtbl while executing the base class constructor. Please also update the copyright year on the files you touched. r=me > Source/bmalloc/ChangeLog:8 > + In the base class of IsoTLSEntry and IsoHeapImplBase, we register each instance to per-process linked-list singleton to /register ... to per-process/register ... with the per-process/. > Source/bmalloc/ChangeLog:16 > + 2. IsoTLSEntry's derived class is initializing the instance including vtable, this happens because base and derived classes have virtual functions. I suggest /vtable/vtable pointer/ because see below ... > Source/bmalloc/ChangeLog:19 > + Then, crash happens because vtable can be in the middle of initialization. IsoHeapImpl has the same problem. I suggest /vtable can be in the middle of initialization/the instance vtable pointer hasn't been set to the derived class' vtable yet/. It isn't the vtable that is being initialized but a pointer to it. I think we should make that clearer. > Source/bmalloc/ChangeLog:24 > + 1. We introduce IsoTLSEntryHolder, which initialize the TLS entry and after fully initialize it, it registers entry to IsoTLSLayout singleton. I suggest rephrasing "entry and after fully initialize it, it registers entry to" as "entry. And after fully initializing it, the holder registers the entry with the". > Source/bmalloc/bmalloc/IsoTLSEntry.cpp:-41 > - IsoTLSLayout::get()->add(this); > - RELEASE_BASSERT(m_offset != UINT_MAX); You can also remove the #include "IsoTLSLayout.h" above. > Source/bmalloc/bmalloc/IsoTLSEntry.h:36 > class IsoTLSLayout; You don't need this anymore.
Comment on attachment 377963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377963&action=review Thanks! >> Source/bmalloc/ChangeLog:8 >> + In the base class of IsoTLSEntry and IsoHeapImplBase, we register each instance to per-process linked-list singleton to > > /register ... to per-process/register ... with the per-process/. Fixed. >> Source/bmalloc/ChangeLog:16 >> + 2. IsoTLSEntry's derived class is initializing the instance including vtable, this happens because base and derived classes have virtual functions. > > I suggest /vtable/vtable pointer/ because see below ... Fixed. >> Source/bmalloc/ChangeLog:19 >> + Then, crash happens because vtable can be in the middle of initialization. IsoHeapImpl has the same problem. > > I suggest /vtable can be in the middle of initialization/the instance vtable pointer hasn't been set to the derived class' vtable yet/. It isn't the vtable that is being initialized but a pointer to it. I think we should make that clearer. Yeah, right. Fixed. >> Source/bmalloc/ChangeLog:24 >> + 1. We introduce IsoTLSEntryHolder, which initialize the TLS entry and after fully initialize it, it registers entry to IsoTLSLayout singleton. > > I suggest rephrasing "entry and after fully initialize it, it registers entry to" as "entry. And after fully initializing it, the holder registers the entry with the". Fixed. >> Source/bmalloc/bmalloc/IsoTLSEntry.cpp:-41 >> - RELEASE_BASSERT(m_offset != UINT_MAX); > > You can also remove the #include "IsoTLSLayout.h" above. Right. Fixed. >> Source/bmalloc/bmalloc/IsoTLSEntry.h:36 >> class IsoTLSLayout; > > You don't need this anymore. Right. Fixed.
Committed r249461: <https://trac.webkit.org/changeset/249461>