[Webkit-unassigned] [Bug 165479] New: JSWrapperMap memory leak

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=165479

            Bug ID: 165479
           Summary: JSWrapperMap memory leak
    Classification: Unclassified
           Product: WebKit
           Version: Other
          Hardware: Unspecified
                OS: Unspecified
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: JavaScriptCore
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: daniel.zimmerman at me.com

Created attachment 296299
  --> https://bugs.webkit.org/attachment.cgi?id=296299&action=review
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).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161206/24127468/attachment.html>