Bug 165479 – JSWrapperMap memory leak

• Status: NEW
• Component: JavaScriptCore

[Dan Zimmerman]

Created attachment 296299 [details]
The xcodeproj containing an example set up where this issue occurs

m_cachedObjCWrappers grows without bound. Even though the JSValue are autoreleased, the map continues to grow as if they were still in existence. The issue appears to be with using a weak valued NSMapTable as described here: http://cocoamine.net/blog/2013/12/13/nsmaptable-and-zeroing-weak-references/

This issue isn't always reproducible - it appears that theres some sort of timing needed to somehow trick the NSMapTable to not resize at the proper time. I ran the app through the leak instrument and it claimed there were "no leaks" and indeed inspecting the live objects nothing appears to be leaking, but NSMapTable appears to continue to grow without bound for some reason (if you look at the logged `count` then you'll see that the count grows very large and then resets back down to a very small number every time the map table is resized, however this doesn't happen often enough so the map table is forced to grow unnecessarily). This does seem like a map table issue, however I think temporarily a fix can be found for JSC.

My proposed solution is to change `m_cachedObjCWrappers` to a `std::unordered_map<JSValueRef, JSValue*>` or whatever equivalent exists in WTF (I'm not very familiar with WTF) and then create a lightweight objc wrapper around JSValueRef that's then associated with the JSValue* and removes the entry in `m_cachedObjCWrappers` when `-dealloc` is called. E.g. the wrapper would look like

```
@interface JSValueRefCacheClearer : NSObject
- (instancetype)initWithValue:(JSValueRef)value;
@end

@implementation JSValueRefCacheClearer {
  JSValueRef _value;
}
- (instancetype)initWithValue:(JSValueRef)value {
  if ((self = [super init]) != nil) {
    _value = value;
  }
  return self;
}

- (void)dealloc {
  m_cachedObjCWrappers.remove(_value);
  [super dealloc];
}
@end
```

And then `-[JSWrapperMap objcWrapperForJSValueRef:]` would change to
```
- (JSValue *)objcWrapperForJSValueRef:(JSValueRef)value {
  JSValue *wrapper = m_cachedObjCWrappers[value];
  if (!wrapper) {
    wrapper = [[[JSValue alloc] initWithValue:value inContext:m_context] autorelease];
    m_cachedObjCWrappers[value] = wrapper;
    JSValueRefCacheClearer *clearer = [[JSValueRefCacheClearer alloc] initWithValue:value];
    objc_setAssociatedObject(wrapper, static_cast<void *>(clearer), clearer, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
    // we pass ownership over to the wrapper
    [clearer release];
  }
  return wrapper;
}
```

I'll gladly create the patch myself (following https://webkit.org/contributing-code/) as long as the bug is accepted as in fact a bug and the proposed change seems reasonable (albeit, it does feel a bit hacky).

[Radar WebKit Bug Importer]

<rdar://problem/30725495>