Bug 141809 – [JSObjCClassInfo reallocateConstructorAndOrPrototype] should also reallocate super class prototype chain

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

[Mark Lam]

A ObjC class that implement the JSExport protocol will have a JS prototype chain and constructor automatically synthesized for its JS wrapper object.  However, if there are no more instances of that ObjC class reachable by a GC root scan, then its synthesized prototype chain and constructors may be released by the GC.  If a new instance of that ObjC class is instantiated, then [JSObjCClassInfo reallocateConstructorAndOrPrototype] should re-construct the prototype chain and constructor (if they were released).  However, the current implementation only re-constructs the immediate prototype, but not every other prototype object upstream in the prototype chain.

The fix is to have [JSObjCClassInfo reallocateConstructorAndOrPrototype] recurse and call itself for the super class if the super class' prototype is not initialized (implying that the super class also needs to re-construct its prototype and constructor).

[Mark Lam]

<rdar://problem/19679557>

[Mark Lam]

Created attachment 246917 [details]
the patch.

[WebKit Commit Bot]

Attachment 246917 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1436:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1440:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1454:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1468:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

(In reply to comment #3)
> Attachment 246917 [details] did not pass style-queue:
> ...
> Total errors found: 4 in 3 files

I think the style bot is just not smart enough to understand these.

[Mark Hahnenberg]

Comment on attachment 246917 [details]
the patch.

r=me. I know it's kind of a PITA, but separating these API tests out into their own files reduces build times and makes it easier to figure out which ones are regressing without the risk of having any dirty state between tests. There's already a couple larger tests/groups of tests that are separate (e.g. JSExportTests.mm). It might make sense to move this out of the monolithic test function and into one of those smaller groups or maybe its own thing.

[Geoffrey Garen]

Comment on attachment 246917 [details]
the patch.

I think this patch is fine, but I think it would be even better to make this code less brittle, since we've had two very similar serious GC bugs in this code.

Is there a way for -classInfoForClass: to always return a full prototype chain, with callers not needing to check for a null one, and also to remove the need for comments and workarounds regarding GC in this file?

For example: Perhaps we can make -classInfoForClass: always return a fully constructed prototype chain in a stack-allocated smart pointer that defers garbage collection. Or, more simply, perhaps callers of -classInfoForClass: should defer garbage collection, and pass the DeferGC pointer to the method in order to prove they have done so. Or -classInfoForClass: could return a pair of <JSObjCClassInfo*, JSC::Strong<JSObject*>>, where the second value was the prototype that needed protection from GC.

[Geoffrey Garen]

Concrete proposal:

(1) -[JSObjCClassInfo initWithContext:...] drops the superClassInfo parameter, and stops calling -allocateConstructorAndPrototypeWithSuperClassInfo:.

(2) Methods of JSObjcClassInfo -- -wrapperForObject:, and -constructor -- do not use the m_ weak data members, and instead call helpers to access those members.

(3) Helpers allocate object graphs if needed and return a JSObject* for the resulting constructor or prototype chain.

(4) Remove all comments about GC. The GC problem is solved by a GC root, which is how GC problems should be solved.

[Mark Lam]

Created attachment 246978 [details]
take 2: with Geoff's and Mark's suggestions applied.

In this patch, -prototype: returns a JSC::JSObject* while -constructor: still returns a JSValue* (which is inconsistent).

Technically, -constructor: plus many other methods and static functions can just work with and return JSC::JSObject* instead of JSValue* as well.  They should defer the allocation of the ObjC JSValue* until the latest possible moment.  However, to keep this patch to its bare minimum, I will defer that change to a later patch.

[WebKit Commit Bot]

Attachment 246978 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:81:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:85:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:99:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:113:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 246978 [details]
take 2: with Geoff's and Mark's suggestions applied.

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

> Source/JavaScriptCore/API/JSWrapperMap.mm:529
> +        [self allocateConstructorAndPrototype];
> +        constructor = m_constructor.get();

I think it's still dangerous to separate the allocate from the load. We want "allocate and load" to be an atomic operation that returns a GC root so that it's impossible for anyone to write a line of code that allocates, GCs, and then loads, causing a error.

The best way to do this is probably to have allocateConstructorAndPrototype return an std::pair<JSObject*, JSObject*> holding the constructor and prototype.

So you would do:

JSC::JSObject* constructor = m_constructor.get();
if (!constructor)
    constructor = [self allocateConstructorAndPrototype].first;

[Mark Lam]

Created attachment 246988 [details]
take 3: with -allocateConstructorAndPrototype returning a prototype constructor pair

[WebKit Commit Bot]

Attachment 246988 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:81:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:85:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:99:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:113:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 246988 [details]
take 3: with -allocateConstructorAndPrototype returning a prototype constructor pair

I'm going to swap the order of the constructor and prototype in the pair to be more intuitive.

[Mark Lam]

Created attachment 246991 [details]
take 4: with consistent constructor and prototype ordering in pair.

[WebKit Commit Bot]

Attachment 246991 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:81:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:85:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:99:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141809.mm:113:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 246991 [details]
take 4: with consistent constructor and prototype ordering in pair.

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

> Source/JavaScriptCore/API/tests/Regress141809.mm:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

Eeek.  Should 2015.  Will fix before commit.

Will also update header for testapi.mm to say 2013-2015, because there were also changes in 2014 though the header only said 2013.
Ditto for JSWrapperMap.mm.

[Geoffrey Garen]

Comment on attachment 246991 [details]
take 4: with consistent constructor and prototype ordering in pair.

r=me

[Mark Lam]

Thanks for the reviews.  Landed in r180452: <http://trac.webkit.org/r180452>.