Bug 221107 – JavaScriptCore should build with ARC enabled

• Component: JavaScriptCore
• Status: NEW

[David Kilzer (:ddkilzer)]

JavaScriptCore should build with ARC enabled.

This bug tracks enabling ARC in JavaScriptCore.
https://bugs.webkit.org/process_bug.cgi?id=216318&addselfcc=1&token=1611874581-tQSFTTFT7FxskGO0_mQG5R26FkHbx02V-VcyNfWsbsw

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #0)
> https://bugs.webkit.org/process_bug.
> cgi?id=216318&addselfcc=1&token=1611874581-tQSFTTFT7FxskGO0_mQG5R26FkHbx02V-
> VcyNfWsbsw

Oops, accidental drag-and-drop of a link.

[Radar WebKit Bug Importer]

<rdar://problem/73726197>

[Darin Adler]

I have a patch that does most of this.

[Darin Adler]

You don’t have to wait for my patch; nothing special in it, and it’s not tested yet. But I will upload soon in case it’s helpful.

[Darin Adler]

Created attachment 418743 [details]
First cut; compiles locally, not tested

[Darin Adler]

Comment on attachment 418743 [details]
First cut; compiles locally, not tested

View in context: https://bugs.webkit.org/attachment.cgi?id=418743&action=review

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.h:68
> +    OSObjectPtr<xpc_connection_t> m_connection;
> +    OSObjectPtr<dispatch_queue_t> m_queue;

Since this header can be included in non-Objective-C .cpp files, I think the safer idiom is to use OSObjectPtr. If we only included this header in Objective-C++ source files, we could instead just rely on ARC directly.

[David Kilzer (:ddkilzer)]

Created attachment 464950 [details]
Patch

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #7)
> Created attachment 464950 [details]
> Patch

This is still a WIP patch.

The following crashes on ASan builds:

$ ./Tools/Scripts/run-javascriptcore-tests --release --no-build --testapi

Like this:

=================================================================
==78457==ERROR: AddressSanitizer: heap-use-after-free on address 0x61100001f160 at pc 0x0001199bb327 bp 0x7ff7b75429e0 sp 0x7ff7b75429d8
READ of size 8 at 0x61100001f160 thread T0
    #0 0x1199bb326 in JSC::WeakImpl::setState(JSC::WeakImpl::State) WeakImpl.h:91
    #1 0x11998cf4d in JSC::WeakSet::deallocate(JSC::WeakImpl*) WeakSet.h:113
    #2 0x11b23a4b8 in JSC::weakClearSlowCase(JSC::WeakImpl*) Weak.cpp:36
    #3 0x119aee841 in JSC::Weak<JSC::JSObject>::clear() Weak.h:86
    #4 0x119aee818 in JSC::Weak<JSC::JSObject>::~Weak() Weak.h:63
    #5 0x119ab8558 in JSC::Weak<JSC::JSObject>::~Weak() Weak.h:62
    #6 0x119ab915a in -[JSObjCClassInfo .cxx_destruct] JSWrapperMap.mm:402
    #7 0x7ff81b89fd0d in object_cxxDestructFromClass(objc_object*, objc_class*)+0x52 (libobjc.A.dylib:x86_64+0x10d0d)
    #8 0x7ff81b89847d in objc_destructInstance+0x5d (libobjc.A.dylib:x86_64+0x947d)
    #9 0x7ff81b89840e in _objc_rootDealloc+0x3d (libobjc.A.dylib:x86_64+0x940e)
    #10 0x119ab3b2e in -[JSObjCClassInfo dealloc] JSWrapperMap.mm:424
    #11 0x7ff81ba768f6 in cow_cleanup+0xa0 (CoreFoundation:x86_64+0x2d8f6)
    #12 0x7ff81ba76810 in -[__NSDictionaryM dealloc]+0x7f (CoreFoundation:x86_64+0x2d810)
    #13 0x7ff81b89c3a9 in AutoreleasePoolPage::releaseUntil(objc_object**)+0xa3 (libobjc.A.dylib:x86_64+0xd3a9)
    #14 0x7ff81b89924e in objc_autoreleasePoolPop+0xa7 (libobjc.A.dylib:x86_64+0xa24e)
    #15 0x108a13b8a in testToString() testapi.mm:2835
    #16 0x108a12a82 in testObjectiveCAPI testapi.mm:2858
    #17 0x1089e86ee in main testapi.c:1408
    #18 0x10da5852d in start+0x1cd (dyld:x86_64+0x552d)

0x61100001f160 is located 224 bytes inside of 256-byte region [0x61100001f080,0x61100001f180)
freed by thread T0 here:
    #0 0x108faaaf6 in __sanitizer_mz_free+0x86 (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x47af6)
    #1 0x118900954 in bmalloc::DebugHeap::free(void*) DebugHeap.cpp:93
    #2 0x118901293 in pas_debug_heap_free DebugHeap.cpp:239
    #3 0x1188f985a in bmalloc_heap_config_specialized_try_deallocate_not_small_exclusive_segregated bmalloc_heap_config.c:43
    #4 0x11bbda288 in WTF::FastMalloc::free(void*) FastMalloc.h:252
    #5 0x11b2465f0 in JSC::WeakBlock::destroy(JSC::Heap&, JSC::WeakBlock*) WeakBlock.cpp:49
    #6 0x11b121f81 in JSC::Heap::~Heap() Heap.cpp:431
    #7 0x11b123198 in JSC::Heap::~Heap() Heap.cpp:419
    #8 0x11c23b168 in JSC::VM::~VM() VM.cpp:462
    #9 0x11c23bff8 in JSC::VM::~VM() VM.cpp:398
    #10 0x119b443e6 in WTF::ThreadSafeRefCounted<JSC::VM, (WTF::DestructionThread)0>::deref() const::'lambda'()::operator()() const ThreadSafeRefCounted.h:115
    #11 0x119b277e3 in WTF::ThreadSafeRefCounted<JSC::VM, (WTF::DestructionThread)0>::deref() const ThreadSafeRefCounted.h:127
    #12 0x11b388520 in WTF::RefPtr<JSC::VM, WTF::RawPtrTraits<JSC::VM>, WTF::DefaultRefDerefTraits<JSC::VM> >::operator=(std::nullptr_t) RefPtr.h:170
    #13 0x11be53399 in JSC::JSLockHolder::~JSLockHolder() JSLock.cpp:73
    #14 0x11be534f8 in JSC::JSLockHolder::~JSLockHolder() JSLock.cpp:71
    #15 0x119b27656 in JSContextGroupRelease JSContextRef.cpp:86
    #16 0x1199b3e87 in -[JSVirtualMachine dealloc] JSVirtualMachine.mm:128
    #17 0x1199d33c8 in WTF::RetainPtr<JSVirtualMachine>::~RetainPtr() RetainPtr.h:178
    #18 0x119999998 in WTF::RetainPtr<JSVirtualMachine>::~RetainPtr() RetainPtr.h:176
    #19 0x119999961 in -[JSContext .cxx_destruct] JSContext.mm:52
    #20 0x7ff81b89fd0d in object_cxxDestructFromClass(objc_object*, objc_class*)+0x52 (libobjc.A.dylib:x86_64+0x10d0d)
    #21 0x7ff81b89847d in objc_destructInstance+0x5d (libobjc.A.dylib:x86_64+0x947d)
    #22 0x7ff81b89840e in _objc_rootDealloc+0x3d (libobjc.A.dylib:x86_64+0x940e)
    #23 0x119997208 in -[JSContext dealloc] JSContext.mm:100
    #24 0x108a13b81 in testToString() testapi.mm:2835
    #25 0x108a12a82 in testObjectiveCAPI testapi.mm:2858
    #26 0x1089e86ee in main testapi.c:1408
    #27 0x10da5852d in start+0x1cd (dyld:x86_64+0x552d)

previously allocated by thread T0 here:
    #0 0x108faa6f0 in __sanitizer_mz_malloc+0xa0 (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x476f0)
    #1 0x7ff81b839aba in _malloc_zone_malloc+0x7c (libsystem_malloc.dylib:x86_64+0x1daba)
    #2 0x118900868 in bmalloc::DebugHeap::malloc(unsigned long, bmalloc::FailureAction) DebugHeap.cpp:72
    #3 0x1189011a8 in pas_debug_heap_malloc DebugHeap.cpp:224
    #4 0x1188cbe11 in pas_debug_heap_allocate pas_debug_heap.h:101
    #5 0x11887dc56 in bmalloc_allocate_impl_casual_case bmalloc_heap_inlines.h:69
    #6 0x11887d998 in bmalloc_allocate_casual bmalloc_heap.c:64
    #7 0x118640fe8 in WTF::FastMalloc::malloc(unsigned long) FastMalloc.h:219
    #8 0x11b24659d in JSC::WeakBlock::create(JSC::Heap&, JSC::CellContainer) WeakBlock.cpp:42
    #9 0x11b2473b8 in JSC::WeakSet::addAllocator(JSC::CellContainer) WeakSet.cpp:112
    #10 0x11b247301 in JSC::WeakSet::findAllocator(JSC::CellContainer) WeakSet.cpp:90
    #11 0x11bee6a64 in JSC::WeakSet::allocate(JSC::JSValue, JSC::WeakHandleOwner*, void*) WeakSetInlines.h:39
    #12 0x119aee7e5 in JSC::Weak<JSC::JSObject>::Weak(JSC::JSObject*, JSC::WeakHandleOwner*, void*) WeakInlines.h:35
    #13 0x119ab8408 in JSC::Weak<JSC::JSObject>::Weak(JSC::JSObject*, JSC::WeakHandleOwner*, void*) WeakInlines.h:36
    #14 0x119ab446f in -[JSObjCClassInfo allocateConstructorAndPrototypeInContext:] JSWrapperMap.mm:525
    #15 0x119ab8cd1 in -[JSObjCClassInfo prototypeInContext:] JSWrapperMap.mm:567
    #16 0x119ab4274 in -[JSObjCClassInfo allocateConstructorAndPrototypeInContext:] JSWrapperMap.mm:520
    #17 0x119ab8cd1 in -[JSObjCClassInfo prototypeInContext:] JSWrapperMap.mm:567
    #18 0x119ab8e41 in -[JSObjCClassInfo structureInContext:] JSWrapperMap.mm:579
    #19 0x119ab8775 in -[JSObjCClassInfo wrapperForObject:inContext:] JSWrapperMap.mm:543
    #20 0x119aba10d in -[JSWrapperMap jsWrapperForObject:inContext:] JSWrapperMap.mm:646
    #21 0x11999a996 in -[JSContext(Internal) wrapperForObjCObject:] JSContext.mm:398
    #22 0x1199b150e in objectToValueWithoutCopy(JSContext*, objc_object*) JSValue.mm:1050
    #23 0x1199a2d96 in objectToValue(JSContext*, objc_object*) JSValue.mm:1057
    #24 0x1199a2b88 in +[JSValue valueWithObject:inContext:] JSValue.mm:96
    #25 0x108a137e1 in testToString() testapi.mm:2817
    #26 0x108a12a82 in testObjectiveCAPI testapi.mm:2858
    #27 0x1089e86ee in main testapi.c:1408
    #28 0x10da5852d in start+0x1cd (dyld:x86_64+0x552d)

SUMMARY: AddressSanitizer: heap-use-after-free WeakImpl.h:91 in JSC::WeakImpl::setState(JSC::WeakImpl::State)


This has to do with the order in which ObjC objects are being deallocated, because making this change to make the `JSContext` object be deallocated last in this function (after any autoreleased objects) will fix the above crash--though there is a second crash that occurs after this one in another test function:

```
 static void testToString()
 {
+    JSContext *context = [[JSContext alloc] init];
     @autoreleasepool {
-        JSContext *context = [[JSContext alloc] init];
         [...]
     }
 }
```

I need help figuring out how to make it okay to deallocate the `JSContext` object before `JSObjCClassInfo` objects.  Or does the test need to change?

[Darin Adler]

Comment on attachment 464950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=464950&action=review

> Source/JavaScriptCore/API/JSValue.mm:78
>          _context = nil;

Not sure we need to keep this.

> Source/JavaScriptCore/API/JSValue.mm:1112
> +    if (!self || !value || !context)
>          return nil;

I don’t think we should call [super init] if value is nullptr or context is nil.

[Darin Adler]

This crash shows that we are doing more autorelease than non-ARC.

That is not good! Has a performance cost as well as making this possible latent ordering problem happen in real life. In particular, it seems that calling classInfoForClass now ends up autoreleasing the JSObjCClassInfo object. It's always returning something that is in the class map NSDictionary, but ARC also wants to retain the local variable because who knows if it will be removed from the dictionary. I wonder if we can rearrange things so that we can call a method on those objects without it ending up in the autorelease pool. It seems strange we end up autoreleasing in this case. I thought ARC was smarter than that and often avoided autorelease on return values for common idioms. Maybe we can find a way to avoid whatever is causing the autorelease to happen.

JSValue is designed to always keep the JSContext alive. But we didn’t think of JSObjCClassInfo as something that had its own lifetime that could outlast the JSWrapperMap we are creating it for.

I think a good fix is either to use unsafe-unretained for JSObjCClassInfo or convert it from an Objective-C class to a C++ class and store it in a HashMap instead of an NSDictionary.

I’d lean toward the "convert to a C++ class". You can’t accidentally autorelease something that isn’t even an Objective-C object.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #10)
> This crash shows that we are doing more autorelease than non-ARC.

I don't think the stack necessarily shows/proves that we're doing more autorelease.

Only a NSMutableDictionary object is being autoreleased in the crashing stack:

    #10 0x119ab3b2e in -[JSObjCClassInfo dealloc] JSWrapperMap.mm:424
    #11 0x7ff81ba768f6 in cow_cleanup+0xa0 (CoreFoundation:x86_64+0x2d8f6)
    #12 0x7ff81ba76810 in -[__NSDictionaryM dealloc]+0x7f (CoreFoundation:x86_64+0x2d810)
    #13 0x7ff81b89c3a9 in AutoreleasePoolPage::releaseUntil(objc_object**)+0xa3 (libobjc.A.dylib:x86_64+0xd3a9)
    #14 0x7ff81b89924e in objc_autoreleasePoolPop+0xa7 (libobjc.A.dylib:x86_64+0xa24e)
    #15 0x108a13b8a in testToString() testapi.mm:2835

I think the next step may be to figure out how this NSMutableDictionary gets autoreleased.

To do this with ARC, we may need to disassemble methods where this NSMutableDictionary is returned and disassemble callers that consume the NSMutableDictionary.

For example, this code in `-classInfoForClass:` makes me wonder what the generated code is doing under ARC vs. MRR (returning an autoreleased ObjC object as a side-effect of an assignment operation--I don't think I've ever seen Objective-C code do this before):

```
         if (!conformsToExportProtocol)
             return m_classMap.get()[(id)cls] = [self classInfoForClass:class_getSuperclass(cls)];
     }
 
-    return m_classMap.get()[(id)cls] = adoptNS([[JSObjCClassInfo alloc] initForClass:cls]).get();
+    return m_classMap.get()[(id)cls] = [[JSObjCClassInfo alloc] initForClass:cls];
 }
```

> JSValue is designed to always keep the JSContext alive. But we didn’t think
> of JSObjCClassInfo as something that had its own lifetime that could outlast
> the JSWrapperMap we are creating it for.
> 
> I think a good fix is either to use unsafe-unretained for JSObjCClassInfo

I think `__weak` would be a better choice than `__unsafe_unretained`.

> or
> convert it from an Objective-C class to a C++ class and store it in a
> HashMap instead of an NSDictionary.
> 
> I’d lean toward the "convert to a C++ class". You can’t accidentally
> autorelease something that isn’t even an Objective-C object.

It would be nice to understand what's happening as we may run into the same issue when switching other projects to ARC.  Rewriting classes isn't always going to be a viable option.

[David Kilzer (:ddkilzer)]

BTW, in the worst case, ASan might disrupt the objc_retainAutoreleasedReturnValue() optimization (that changes autoreleased objects into +1 retained references), so we need to consider that might be happening as well.  Compiling without ASan would tell us if that's an issue.

[David Kilzer (:ddkilzer)]

This patch fixes the autorelease of the `NSMutableDictionary` holding `JSObjCClassInfo` objects (the first crash I note in Comment #8), and the next crash is an autoreleased `JSObjCClassInfo` (perhaps as you suggested above):

```
diff --git a/Source/JavaScriptCore/API/JSWrapperMap.mm b/Source/JavaScriptCore/API/JSWrapperMap.mm
index 33980dc736e0..9d04279e7147 100644
--- a/Source/JavaScriptCore/API/JSWrapperMap.mm
+++ b/Source/JavaScriptCore/API/JSWrapperMap.mm
@@ -585,7 +585,7 @@ - (JSC::Structure*)structureInContext:(JSContext *)context
 @end
 
 @implementation JSWrapperMap {
-    RetainPtr<NSMutableDictionary> m_classMap;
+    NSMutableDictionary<Class, JSObjCClassInfo *> *m_classMap;
     std::unique_ptr<JSC::WeakGCMap<__unsafe_unretained id, JSC::JSObject>> m_cachedJSWrappers;
     RetainPtr<NSMapTable> m_cachedObjCWrappers;
 }
@@ -604,7 +604,7 @@ - (instancetype)initWithGlobalContextRef:(JSGlobalContextRef)context
 
     ASSERT(!toJSGlobalObject(context)->wrapperMap());
     toJSGlobalObject(context)->setWrapperMap(self);
-    m_classMap = adoptNS([[NSMutableDictionary alloc] init]);
+    m_classMap = [[NSMutableDictionary alloc] init];
     return self;
 }
 
@@ -614,7 +614,7 @@ - (JSObjCClassInfo*)classInfoForClass:(Class)cls
         return nil;
 
     // Check if we've already created a JSObjCClassInfo for this Class.
-    if (JSObjCClassInfo* classInfo = (JSObjCClassInfo*)m_classMap.get()[(id)cls])
+    if (JSObjCClassInfo* classInfo = m_classMap[cls])
         return classInfo;
 
     // Skip internal classes beginning with '_' - just copy link to the parent class's info.
@@ -626,10 +626,10 @@ - (JSObjCClassInfo*)classInfoForClass:(Class)cls
         });
 
         if (!conformsToExportProtocol)
-            return m_classMap.get()[(id)cls] = [self classInfoForClass:class_getSuperclass(cls)];
+            return m_classMap[cls] = [self classInfoForClass:class_getSuperclass(cls)];
     }
 
-    return m_classMap.get()[(id)cls] = [[JSObjCClassInfo alloc] initForClass:cls];
+    return m_classMap[cls] = [[JSObjCClassInfo alloc] initForClass:cls];
 }
 
 - (JSValue *)jsWrapperForObject:(id)object inContext:(JSContext *)context
```

This makes me wonder how code is generated for RetainPtr::get() under ARC, and whether we need to make a fix there.

Specifically, ARC might work best if RetainPtr::get() explicitly returned a retained+autoreleased object (for example, by the compiler generating an objc_autoreleaseReturnValue() call), so that the caller can use objc_retainAutoreleasedReturnValue() to convert the autoreleased object into a +1 retained object.

Just a theory at this point.

[David Kilzer (:ddkilzer)]

This "fixes" the autoreleased `JSObjCClassInfo` object in `-jsWrapperForObject:inContext:`:

```
     if (class_isMetaClass(object_getClass(object)))
         jsWrapper = [[self classInfoForClass:(Class)object] constructorInContext:context];
     else {
+@autoreleasepool {
         JSObjCClassInfo* classInfo = [self classInfoForClass:[object class]];
         jsWrapper = [classInfo wrapperForObject:object inContext:context];
+}
     }
```

The next crash is caused by an autoreleased `JSVirtualMachine` object.

[David Kilzer (:ddkilzer)]

> The next crash is caused by an autoreleased `JSVirtualMachine` object.

This fixes the autoreleased `JSVirtualMachine`:

```
 + (JSVirtualMachine *)virtualMachineWithContextGroupRef:(JSContextGroupRef)group
 {
-    JSVirtualMachine *virtualMachine = [JSVMWrapperCache wrapperForJSContextGroupRef:group];
+    JSVirtualMachine *virtualMachine;
+@autoreleasepool {
+    virtualMachine = [JSVMWrapperCache wrapperForJSContextGroupRef:group];
+}
     if (!virtualMachine)
         virtualMachine = [[JSVirtualMachine alloc] initWithContextGroupRef:group];
     return virtualMachine;
 }
```

And, of course, the next crash is an autoreleased `JSContext` object.

(The `JSVirtualMachine` and `JSContext` crashes are in `testLoaderResolvesAbsoluteScriptURL()` now, not `testToString()`.)

[David Kilzer (:ddkilzer)]

> And, of course, the next crash is an autoreleased `JSContext` object.

The autoreleased `JSContext` object is mitigated by:

```
 + (JSContext *)contextWithJSGlobalContextRef:(JSGlobalContextRef)globalContext
 {
     JSVirtualMachine *virtualMachine = [JSVirtualMachine virtualMachineWithContextGroupRef:toRef(&toJS(globalContext)->vm())];
-    JSContext *context = [virtualMachine contextForGlobalContextRef:globalContext];
+    JSContext *context;
+@autoreleasepool {
+    context = [virtualMachine contextForGlobalContextRef:globalContext];
+}
     if (!context)
         context = [[JSContext alloc] initWithGlobalContextRef:globalContext];
     return context;
 }
```

Next crash is due to an autoreleased `JSValue` object, still running `testLoaderResolvesAbsoluteScriptURL()`.

Are we getting close to the end?  I'm trying to get the tests working, then go back to figure out why objects are being autoreleased in each of these cases.

[David Kilzer (:ddkilzer)]

There are too many places where a `JSValue` can be created, so I'm going to change strategies to pause execution in `testapi.mm` at the end of an autoreleasePool (ARP), then use MallocStackLogging and `leaks` (a) to list the objects in the ARP, and (b) to get stack traces for the allocation of those objects so I know where they're coming from.

[Darin Adler]

I’m glad you are continuing to analyze and look deeply at what is happening here.

But I suggest you think about what our eventual goal is for for the autoreleased objects?

If we don’t want autoreleased objects to leak out, then we can put autorelease pools at all the API boundaries.

If we don’t want to pay the increased performance cost of putting more objects into the autorelease pool, then we probably can’t accomplish that by adding @autoreleasepool. One approach is to change idioms to not use Objective-C for things that we don’t need to and cut down on the use of RetainPtr for Objective-C objects. RetainPtr offers interoperability between ARC and non-ARC code, but I believe that comes at the cost of autorelease. It’s good for safely putting an Objective-C object into a header that can be included by ARC and non-ARC code (possibly including non-Objective-C C++ code), but isn’t really needed in code that’s only used with ARC. Certainly not in private implementation details of Objective-C classes.

[Darin Adler]

Note that even before ARC we would definitely autorelease JSValue objects, including as part of the public API contract. That’s one reason JSValue objects are careful to take care of their lifetimes properly.

So we don’t need to get rid of JSValue autoreleases just to keep the API contract clean the way we do with internal objects like the JSObjCClassInfo objects.

But we still might want to make sure there are not a lot more than before because of the additional performance cost. I suppose ideally if the caller is using Swift or ARC Objective-C maybe we could get to "no autorelease at all", although I am not sure that is possible.

If we take all of this on, it might be too big a project.

I think the "autorelease that breaks semantics" problem with JSObjCClassInfo is a different type of problem than the JSValue part.

[David Kilzer (:ddkilzer)]

Another PR was posted here:  https://github.com/WebKit/WebKit/pull/11299

[David Kilzer (:ddkilzer)]

Created attachment 474320 [details]
Patch v3

Just to run tests; not to land.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #21)
> Created attachment 474320 [details]
> Patch v3

This rebases the PR onto 290921@main along with the required fixes to compile.

Not ready to land yet as JSC API tests still crash (more reproducible with ASan builds):

$ ./Tools/Scripts/run-javascriptcore-tests --release --no-build --testapi