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.
<rdar://problem/16625128>
Created attachment 229841 [details] Patch
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.
Created attachment 229847 [details] Patch
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 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?
Created attachment 229854 [details] Fix build
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.
Created attachment 229856 [details] Fix build
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.
Created attachment 229887 [details] Fix build
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.
Created attachment 229890 [details] Fix build
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.
Created attachment 229918 [details] Fix build
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.
Created attachment 229933 [details] I have a dream
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.
Created attachment 229941 [details] that one day
Created attachment 229943 [details] we will live in a world
Created attachment 229944 [details] where a patch
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.
Created attachment 229946 [details] will be judged
Comment on attachment 229946 [details] will be judged is this patch still relevant?
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.