Bug 194031 – [JSC] UnlinkedMetadataTable assumes that MetadataTable is destroyed before it is destructed, but order of destruction of JS heap cells are not guaranteed

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

[Yusuke Suzuki]

If your UnlinkedCodeBlock is destructed before linked CodeBlock is destructed, we have a bad time.

[Yusuke Suzuki]

Created attachment 361003 [details]
Patch

[Yusuke Suzuki]

Created attachment 361004 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 361004 [details]
Patch

I'll do a bit more cleanup.

[Yusuke Suzuki]

Created attachment 361005 [details]
Patch

[EWS Watchlist]

Comment on attachment 361005 [details]
Patch

Attachment 361005 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11014368

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

Created attachment 361009 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

(In reply to Build Bot from comment #5)
> Comment on attachment 361005 [details]
> Patch
> 
> Attachment 361005 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/11014368
> 
> New failing tests:
> imported/w3c/web-platform-tests/webrtc/simplecall.https.html

I've checked the results and it seems unrelated to this patch. Just flaky.

[Saam Barati]

Comment on attachment 361005 [details]
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.

[Yusuke Suzuki]

Comment on attachment 361005 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/MetadataTable.cpp:62
>> +    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.

The possible cleanup would be defining a static function MetadataTable::destroy and call it from deref().

void MetadataTable::destroy(MetadataTable& table)
{
    Ref<UnlinkedMetadataTable> unlinkedMetadata = WTFMove(table.linkingData().unlinkedMetadata);
    table.~MetadataTable();
    unlinkedMetadata->unlink(table};
}

I would like to do that thing in a different patch since this solves another problem in MetadataTable.

[Yusuke Suzuki]

Committed r240915: <https://trac.webkit.org/changeset/240915>

[Radar WebKit Bug Importer]

<rdar://problem/47779358>