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.
<rdar://problem/30387376>
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.
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.
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?
I'm not too worried about the compatibility risk to these changes.
Created attachment 310677 [details] Patch
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.
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.
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.
Created attachment 310895 [details] Patch for landing
Comment on attachment 310895 [details] Patch for landing Clearing flags on attachment: 310895 Committed r217240: <http://trac.webkit.org/changeset/217240>
All reviewed patches have been landed. Closing bug.
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.
> I'll look into cleaning this up. Bug 173110.