Bug 131691 - JSVirtualMachine could disappear and take its external object graph with it
Summary: JSVirtualMachine could disappear and take its external object graph with it
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-15 13:21 PDT by Mark Hahnenberg
Modified: 2016-09-17 07:16 PDT (History)
6 users (show)

See Also:


Attachments
Patch (48.49 KB, patch)
2014-04-21 17:15 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (48.52 KB, patch)
2014-04-21 18:37 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix build (48.46 KB, patch)
2014-04-21 20:01 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix build (48.44 KB, patch)
2014-04-21 20:34 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix build (48.45 KB, patch)
2014-04-22 09:20 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix build (48.51 KB, patch)
2014-04-22 09:47 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix build (47.79 KB, patch)
2014-04-22 16:14 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
I have a dream (48.21 KB, patch)
2014-04-22 17:50 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
that one day (48.68 KB, patch)
2014-04-22 19:08 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
we will live in a world (48.71 KB, patch)
2014-04-22 19:11 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
where a patch (47.55 KB, patch)
2014-04-22 19:25 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
will be judged (54.88 KB, patch)
2014-04-22 19:42 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2014-04-15 13:21:42 PDT
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.
Comment 1 Radar WebKit Bug Importer 2014-04-15 14:08:28 PDT
<rdar://problem/16625128>
Comment 2 Mark Hahnenberg 2014-04-21 17:15:16 PDT
Created attachment 229841 [details]
Patch
Comment 3 WebKit Commit Bot 2014-04-21 17:16:46 PDT
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.
Comment 4 Mark Hahnenberg 2014-04-21 18:37:41 PDT
Created attachment 229847 [details]
Patch
Comment 5 WebKit Commit Bot 2014-04-21 18:38:52 PDT
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.
Comment 6 Darin Adler 2014-04-21 18:51:34 PDT
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?
Comment 7 Mark Hahnenberg 2014-04-21 20:01:46 PDT
Created attachment 229854 [details]
Fix build
Comment 8 WebKit Commit Bot 2014-04-21 20:04:27 PDT
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.
Comment 9 Mark Hahnenberg 2014-04-21 20:34:32 PDT
Created attachment 229856 [details]
Fix build
Comment 10 WebKit Commit Bot 2014-04-21 20:35:41 PDT
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.
Comment 11 Mark Hahnenberg 2014-04-22 09:20:06 PDT
Created attachment 229887 [details]
Fix build
Comment 12 WebKit Commit Bot 2014-04-22 09:22:52 PDT
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.
Comment 13 Mark Hahnenberg 2014-04-22 09:47:18 PDT
Created attachment 229890 [details]
Fix build
Comment 14 WebKit Commit Bot 2014-04-22 09:48:47 PDT
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.
Comment 15 Mark Hahnenberg 2014-04-22 16:14:17 PDT
Created attachment 229918 [details]
Fix build
Comment 16 WebKit Commit Bot 2014-04-22 16:15:30 PDT
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.
Comment 17 Mark Hahnenberg 2014-04-22 17:50:54 PDT
Created attachment 229933 [details]
I have a dream
Comment 18 WebKit Commit Bot 2014-04-22 17:53:04 PDT
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.
Comment 19 Mark Hahnenberg 2014-04-22 19:08:07 PDT
Created attachment 229941 [details]
that one day
Comment 20 Mark Hahnenberg 2014-04-22 19:11:21 PDT
Created attachment 229943 [details]
we will live in a world
Comment 21 Mark Hahnenberg 2014-04-22 19:25:08 PDT
Created attachment 229944 [details]
where a patch
Comment 22 WebKit Commit Bot 2014-04-22 19:26:57 PDT
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.
Comment 23 Mark Hahnenberg 2014-04-22 19:42:16 PDT
Created attachment 229946 [details]
will be judged
Comment 24 Oliver Hunt 2014-08-18 13:40:56 PDT
Comment on attachment 229946 [details]
will be judged

is this patch still relevant?
Comment 25 Michael Catanzaro 2016-09-17 07:16:24 PDT
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.