[webkit-reviews] review granted: [Bug 194031] [JSC] UnlinkedMetadataTable assumes that MetadataTable is destroyed before it is destructed, but order of destruction of JS heap cells are not guaranteed : [Attachment 361005] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 194031: [JSC] UnlinkedMetadataTable assumes that MetadataTable is destroyed
before it is destructed, but order of destruction of JS heap cells are not
guaranteed
https://bugs.webkit.org/show_bug.cgi?id=194031

Attachment 361005: Patch

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




--- Comment #8 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 361005
  --> https://bugs.webkit.org/attachment.cgi?id=361005
Patch

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

> Source/JavaScriptCore/bytecode/MetadataTable.cpp:62
> +    Ref<UnlinkedMetadataTable> unlinkedMetadata =
WTFMove(linkingData().unlinkedMetadata);
> +    linkingData().~LinkingData();
> +    // Since UnlinkedMetadata::unlink frees the underlying memory of
MetadataTable.
> +    // We need to destroy LinkingData before calling it.
> +    unlinkedMetadata->unlink(*this);

This whole dance is really poorly abstracted. It's very weird that
UnlinkedMetadataTable frees the memory of MetadataTable. Logically, it would be
incorrect for somebody to add a field to MetadataTable that gets auto destroyed
since that'd mean the memory here is freed before the destructor runs.