[webkit-reviews] review granted: [Bug 201448] [bmalloc] IsoTLSLayout and AllIsoHeaps registration is racy with derived class initialization with virtual functions : [Attachment 377963] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 201448: [bmalloc] IsoTLSLayout and AllIsoHeaps registration is racy with
derived class initialization with virtual functions
https://bugs.webkit.org/show_bug.cgi?id=201448

Attachment 377963: Patch

https://bugs.webkit.org/attachment.cgi?id=377963&action=review




--- Comment #7 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 377963
  --> https://bugs.webkit.org/attachment.cgi?id=377963
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.