We do not use Objective-C weak semantics here. Objective-C JSValue has a dealloc function, and JSValue is tightly coupled to JSContext (it has _context field). So JSValue is only stored in one wrapper map held by JSContext (JSWrapperMap). So, we can manage lifetime just unregistering itself from JSWrapperMap in [JSValue dealloc]. Why we would like to avoid Objective-C weak semantics is that it allocates much memory. In very simple hello_world case, we create 4 JSValue wrappers, and it consumes 10KB for weak semantics.
Created attachment 366254 [details] Patch WIP
Created attachment 366320 [details] Patch
Comment on attachment 366320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366320&action=review > Source/JavaScriptCore/API/JSContext.mm:374 > + JSC::JSLockHolder locker(toJS(m_context)); It is not necessary since we just drop Hash table entry, which contains a pointer, and do nothing on this pointer. I'll remove this lock.
Comment on attachment 366320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366320&action=review > Source/JavaScriptCore/API/JSWrapperMap.mm:604 > + void* m_wrapper { nullptr }; As you mentioned, let's use __unsafe_unretained
Comment on attachment 366320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366320&action=review Thanks! >> Source/JavaScriptCore/API/JSWrapperMap.mm:604 >> + void* m_wrapper { nullptr }; > > As you mentioned, let's use __unsafe_unretained Fixed.
Committed r243672: <https://trac.webkit.org/changeset/243672>
<rdar://problem/49443020>
Re-opened since this is blocked by bug 196952
I rolled out this patch for now. The problem was this patch does not account that JSC.framework is thread-safe. JSC VM can be used multiple threads, and JSC framework takes appropriate lock internally. And [JSValue release] can be called in one thread while the other thread uses it. So the problem happens when, 1. Thread A calls [JSValue release]. 2. Thread A is now in [JSValue dealloc], and about to unregister JSValue from wrapper map. 3. Thread B gets the JSValue from the wrapper map. 4. Thread A unregisters the JSValue, and destroy it. 5. Thread B uses the destroyed JSValue. Appropriate fix in this patch's direction would be like, defining custom [JSValue release] and unregister it from wrapper map and destroy it in an atomic manner. But since this introduces a lot of complexity, for now, I've just reverted this patch.