JSVirtualMachine could disappear and take its external object graph with it

• Component: JavaScriptCore
• Status: NEW ---

[Mark Hahnenberg]

JSVirtualMachine is a thin Objective-C wrapper around JSC::VM. It is also used to store the external graph of Objective-C object references that JSC will use during garbage collection to determine object liveness. Because these JSVirtualMachine wrappers are cached in a weak map, they could disappear even though the VM is still alive taking their external object graphs with them. This could lead to premature reclamation, which is bad. We should fix this.

[Radar WebKit Bug Importer]

<rdar://problem/16625128>

[Mark Hahnenberg]

Created attachment 229841 [details]
Patch

[WebKit Commit Bot]

Attachment 229841 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 229847 [details]
Patch

[WebKit Commit Bot]

Attachment 229847 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 229847 [details]
Patch

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

> Source/JavaScriptCore/API/APIData.h:36
> +class APIData {

Maybe this can just be a struct rather than having getters and setters.

> Source/JavaScriptCore/API/APIData.h:40
> +    APIData()
> +    {
> +    }

Should just omit this. Generated one should be fine.

> Source/JavaScriptCore/API/APIData.h:62
> +    RetainPtr<CFTypeRef> m_externalObjectGraph;
> +    RetainPtr<CFTypeRef> m_externalRememberedSet;

Why so coy about the fact that the type is NSMapTable? Is there a problem compiling that when it’s non-Objective-C++?

> Source/JavaScriptCore/API/JSVirtualMachine.mm:118
> +        vm.apiData.setExternalObjectGraph(RetainPtr<CFTypeRef>(
> +            [[NSMapTable alloc] initWithKeyOptions:weakIDOptions valueOptions:strongIDOptions capacity:0]));

This leaks. It needs to be adoptNS, not just a conversion to RetainPtr.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:125
> +        vm.apiData.setExternalRememberedSet(RetainPtr<CFTypeRef>(
> +            [[NSMapTable alloc] initWithKeyOptions:weakIDOptions valueOptions:integerOptions capacity:0]));

Ditto.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:187
> +    NSMapTable *externalObjectGraph = static_cast<id>(vm.apiData.externalObjectGraph());

Why is this one casting to <id>, but all the others casting to NSMapTable?

> Source/JavaScriptCore/runtime/VM.h:213
> +        APIData apiData;

Why so high in the class? Is this in some sort of order?

[Mark Hahnenberg]

Created attachment 229854 [details]
Fix build

[WebKit Commit Bot]

Attachment 229854 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 229856 [details]
Fix build

[WebKit Commit Bot]

Attachment 229856 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 229887 [details]
Fix build

[WebKit Commit Bot]

Attachment 229887 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/APIData.h:26:  #ifndef header guard has wrong style, please use: APIData_h  [build/header_guard] [5]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 229890 [details]
Fix build

[WebKit Commit Bot]

Attachment 229890 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/APIData.h:26:  #ifndef header guard has wrong style, please use: APIData_h  [build/header_guard] [5]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 229918 [details]
Fix build

[WebKit Commit Bot]

Attachment 229918 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/APIData.h:26:  #ifndef header guard has wrong style, please use: APIData_h  [build/header_guard] [5]
Total errors found: 5 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 229933 [details]
I have a dream

[WebKit Commit Bot]

Attachment 229933 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/APIData.h:26:  #ifndef header guard has wrong style, please use: APIData_h  [build/header_guard] [5]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 229941 [details]
that one day

[Mark Hahnenberg]

Created attachment 229943 [details]
we will live in a world

[Mark Hahnenberg]

Created attachment 229944 [details]
where a patch

[WebKit Commit Bot]

Attachment 229944 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/APIData.h:26:  #ifndef header guard has wrong style, please use: APIData_h  [build/header_guard] [5]
Total errors found: 5 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 229946 [details]
will be judged

[Oliver Hunt]

Comment on attachment 229946 [details]
will be judged

is this patch still relevant?

[Michael Catanzaro]

Comment on attachment 229946 [details]
will be judged

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.