NEW 131691
JSVirtualMachine could disappear and take its external object graph with it
https://bugs.webkit.org/show_bug.cgi?id=131691
Summary JSVirtualMachine could disappear and take its external object graph with it
Mark Hahnenberg
Reported 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.
Attachments
Patch (48.49 KB, patch)
2014-04-21 17:15 PDT, Mark Hahnenberg
no flags
Patch (48.52 KB, patch)
2014-04-21 18:37 PDT, Mark Hahnenberg
no flags
Fix build (48.46 KB, patch)
2014-04-21 20:01 PDT, Mark Hahnenberg
no flags
Fix build (48.44 KB, patch)
2014-04-21 20:34 PDT, Mark Hahnenberg
no flags
Fix build (48.45 KB, patch)
2014-04-22 09:20 PDT, Mark Hahnenberg
no flags
Fix build (48.51 KB, patch)
2014-04-22 09:47 PDT, Mark Hahnenberg
no flags
Fix build (47.79 KB, patch)
2014-04-22 16:14 PDT, Mark Hahnenberg
no flags
I have a dream (48.21 KB, patch)
2014-04-22 17:50 PDT, Mark Hahnenberg
no flags
that one day (48.68 KB, patch)
2014-04-22 19:08 PDT, Mark Hahnenberg
no flags
we will live in a world (48.71 KB, patch)
2014-04-22 19:11 PDT, Mark Hahnenberg
no flags
where a patch (47.55 KB, patch)
2014-04-22 19:25 PDT, Mark Hahnenberg
no flags
will be judged (54.88 KB, patch)
2014-04-22 19:42 PDT, Mark Hahnenberg
no flags
Radar WebKit Bug Importer
Comment 1 2014-04-15 14:08:28 PDT
Mark Hahnenberg
Comment 2 2014-04-21 17:15:16 PDT
WebKit Commit Bot
Comment 3 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.
Mark Hahnenberg
Comment 4 2014-04-21 18:37:41 PDT
WebKit Commit Bot
Comment 5 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.
Darin Adler
Comment 6 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?
Mark Hahnenberg
Comment 7 2014-04-21 20:01:46 PDT
Created attachment 229854 [details] Fix build
WebKit Commit Bot
Comment 8 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.
Mark Hahnenberg
Comment 9 2014-04-21 20:34:32 PDT
Created attachment 229856 [details] Fix build
WebKit Commit Bot
Comment 10 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.
Mark Hahnenberg
Comment 11 2014-04-22 09:20:06 PDT
Created attachment 229887 [details] Fix build
WebKit Commit Bot
Comment 12 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.
Mark Hahnenberg
Comment 13 2014-04-22 09:47:18 PDT
Created attachment 229890 [details] Fix build
WebKit Commit Bot
Comment 14 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.
Mark Hahnenberg
Comment 15 2014-04-22 16:14:17 PDT
Created attachment 229918 [details] Fix build
WebKit Commit Bot
Comment 16 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.
Mark Hahnenberg
Comment 17 2014-04-22 17:50:54 PDT
Created attachment 229933 [details] I have a dream
WebKit Commit Bot
Comment 18 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.
Mark Hahnenberg
Comment 19 2014-04-22 19:08:07 PDT
Created attachment 229941 [details] that one day
Mark Hahnenberg
Comment 20 2014-04-22 19:11:21 PDT
Created attachment 229943 [details] we will live in a world
Mark Hahnenberg
Comment 21 2014-04-22 19:25:08 PDT
Created attachment 229944 [details] where a patch
WebKit Commit Bot
Comment 22 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.
Mark Hahnenberg
Comment 23 2014-04-22 19:42:16 PDT
Created attachment 229946 [details] will be judged
Oliver Hunt
Comment 24 2014-08-18 13:40:56 PDT
Comment on attachment 229946 [details] will be judged is this patch still relevant?
Michael Catanzaro
Comment 25 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.
Note You need to log in before you can comment on or make changes to this bug.