WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2014-04-15 14:08:28 PDT
<
rdar://problem/16625128
>
Mark Hahnenberg
Comment 2
2014-04-21 17:15:16 PDT
Created
attachment 229841
[details]
Patch
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
Created
attachment 229847
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug