Bug 201448 – [bmalloc] IsoTLSLayout and AllIsoHeaps registration is racy with derived class initialization with virtual functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[bmalloc] IsoTLSLayout and AllIsoHeaps registration is racy with derived class initialization with virtual functions

[Yusuke Suzuki]

<rdar://problem/55007731>

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Created attachment 377960 [details]
Patch

[Yusuke Suzuki]

I'll quickly fix GTK EWS failure.

[Yusuke Suzuki]

Created attachment 377962 [details]
Patch

[Yusuke Suzuki]

Created attachment 377963 [details]
Patch

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r249461: <https://trac.webkit.org/changeset/249461>