Bug 167708 – [Cocoa] An exported Objective C class’s prototype and constructor don't persist across JSContext deallocation

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

If a class A with an initializer is exported to JavaScriptCore, and its constructor is referenced by a global object in some context, but the JSContext wrapping that context get deallocated, then a new, different constructor is created the next time it’s needed.

To see the problem, build and run this program:

@import JavaScriptCore;
@import Foundation;

@protocol AJSExport <JSExport>
- (instancetype)init;
@end

@interface A : NSObject <AJSExport>
@end

@implementation A
@end

int main(int argc, const char * argv[]) {
    JSGlobalContextRef contextRef;
    @autoreleasepool {
        JSContext *context = [[JSContext alloc] init];
        contextRef = JSGlobalContextRetain(context.JSGlobalContextRef);
        context[@"A"] = A.class;
        NSLog(@"%@", [context evaluateScript:@"new A().constructor === A"]);
    }

    @autoreleasepool {
        JSContext *context = [JSContext contextWithJSGlobalContextRef:contextRef];
        NSLog(@"%@", [context evaluateScript:@"new A().constructor === A"]);
    }

    JSGlobalContextRelease(contextRef);

    return 0;
}

The first NSLog statement logs true, and the second one logs false. It’s expected to log true as well.

[Radar WebKit Bug Importer]

<rdar://problem/30387376>

[Geoffrey Garen]

Note that this problem would reproduce with any methodology for passing an ObjC object to JS -- it is incidental in this example that we allocate the ObjC object using an ObjC constructor.

[Geoffrey Garen]

The cause of this bug is that we tie the lifetime of JSWrapperMap to the lifetime of JSContext. Instead, we need to tie the lifetime of JSWrapperMap to the GC lifetime of a given JSGlobalObject.

[Keith Miller]

I would propose a different solution to fix this. I think we should just set the .prototype property on A and make it read-only and non-configurable. Then we can run the same code for constructing ObjC classes as we do for host constructors. This also has the nice property that it does what people would expect from JS.

There are a couple of behavior changes to consider with this proposal, however: 

1) If you set the .prototype property on A, right now it will work but do nothing. This change will make code that relies on changing the .prototype property 

2) If you make two constructors for your class both share the same .prototype. This change would make them have different prototypes, which seems closer to what one would expect from JS.

3) If you extend (in JS) your constructor, any super() calls don't respect new.target. This change would make it respect new.target. (I didn't test this but the API code doesn't call createSubclassStructure so I assume it doesn't)

The only thing I would worry about causing incompatible changes is 2. I could totally imagine someone making two constructors, say A and B, for their class and only setting properties on A's .prototype. Currently instances of B will still see A's .prototype properties since both constructors will share the same .prototype object.

I'm not sure if that's something we should worry about. I would guess most users only put their constructors on the global object. Geoff, do you have any insight here?

[Geoffrey Garen]

I'm not too worried about the compatibility risk to these changes.

[Keith Miller]

Created attachment 310677 [details]
Patch

[Mark Lam]

Comment on attachment 310677 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGlobalObject.h:52
> +OBJC_CLASS JSWrapperMap;

Need to wrap with #if JSC_OBJC_API_ENABLED.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:831
> +    JSWrapperMap* wrapperMap() const { return m_wrapperMap.get(); }
> +    void setWrapperMap(JSWrapperMap* map) { m_wrapperMap = map; }

Ditto.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:862
> +    RetainPtr<JSWrapperMap> m_wrapperMap;

Ditto.

[Geoffrey Garen]

Comment on attachment 310677 [details]
Patch

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

Mark is right about JSC_OBJC_API_ENABLED -- that's the GTK build failure.

> Source/JavaScriptCore/ChangeLog:11
> +        Also, this patch fixes a "bug" where we would not just used the object constructor/prototype for NSObject.

NSObject doesn't export anything to JavaScript. Is this change observable? If so, we should add a test for it.

[Geoffrey Garen]

Comment on attachment 310677 [details]
Patch

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

> Source/JavaScriptCore/API/JSWrapperMap.mm:476
> -        if (!jsPrototype) {
> -            JSValue *prototype = constructor[@"prototype"];
> -            jsPrototype = toJS(JSValueToObject(cContext, valueInternalValue(prototype), 0));
> -        }
> +        if (!jsPrototype)
> +            jsPrototype = globalObject->objectPrototype();

A better way to describe this, rather than saying "we would not just used the object constructor/prototype for NSObject", is to fully describe the edge case, where we would do the wrong thing if and only if the user had overwritten the window.Object property.

[Keith Miller]

Created attachment 310895 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 310895 [details]
Patch for landing

Clearing flags on attachment: 310895

Committed r217240: <http://trac.webkit.org/changeset/217240>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 310895 [details]
Patch for landing

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

> Source/JavaScriptCore/API/JSContext.mm:57
> +        [[JSWrapperMap alloc] initWithGlobalContextRef:[self JSGlobalContextRef]];

This looks like it will leak the JSWrapperMap.

- This alloc produces a +1 instance
- Inside the initializer, the JSGlobalObject is made to retain the wrapper via a RetainPtr<JSWrapperMap>
- However nobody ends up releasing this +1. It seems this code should release or autorelease this new instance and probably include a healthy comment about the non-obvious lifetime

I'll look into cleaning this up.

[Joseph Pecoraro]

> I'll look into cleaning this up.

Bug 173110.