Bug 122351 – JSManagedValue should be able to store non-object JSValues

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

[Mark Hahnenberg]

We decided not to support this because we thought it didn’t make sense to have a “weak” JSValue that wasn’t an object.

Our general thought process was if you have a JSObject-ObjC object pair (i.e. an Obj-C object that you exported to JavaScript-land), it makes more sense to store a non-object JSValue on the JavaScript-land version of the object rather than as an ivar in the Objective-C object.

In retrospect, this may not have been a good decision at least w.r.t. consistency in client code. If you’re storing a bag of JSValues off an Obj-C object, you’d like to store all of them either in ObjC-land or JavaScript-land, but doing some in one and some in the other doesn’t sound too good. Also, what if the object you want to hang these values off of doesn’t have a corresponding object in JavaScript-land in which to store them?

I think the solution would probably be to fix JSManagedValue to be able to reference non-object JSValues. Right now, all JSManagedValues contain a Weak<JSObject>. 

To fix this, we should change Weak<T> to allow non-object template parameters (similar to how WriteBarrier has an “Unknown” template specialization). The downside of this option is that it's a tad wasteful, as we’ll allocate some additional, unnecessary state in the JS runtime. The upside is that the virtual machine will clear the Weak field when it goes away, preventing clients from accidentally trying to do something with it later.

[Mark Hahnenberg]

<rdar://problem/15151935>

[Mark Hahnenberg]

Created attachment 213416 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSManagedValue.mm', u'Source/JavaScriptCore/API/tests/testapi.mm', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/heap/Heap.cpp', u'Source/JavaScriptCore/heap/Heap.h', u'Source/JavaScriptCore/heap/Weak.h', u'Source/JavaScriptCore/heap/WeakBlock.cpp', u'Source/JavaScriptCore/heap/WeakImpl.h', u'Source/JavaScriptCore/heap/WeakInlines.h', u'Source/JavaScriptCore/heap/WeakSet.h', u'Source/JavaScriptCore/heap/WeakSetInlines.h', u'Source/JavaScriptCore/runtime/VM.h']" exit_code: 1
Source/JavaScriptCore/heap/Weak.h:64:  Missing spaces around &&  [whitespace/operators] [3]
Source/JavaScriptCore/heap/Weak.h:73:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 2 in 12 files


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

[Build Bot]

Comment on attachment 213416 [details]
Patch

Attachment 213416 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3314041

[Mark Hahnenberg]

Created attachment 213452 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSManagedValue.mm', u'Source/JavaScriptCore/API/tests/testapi.mm', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/heap/Heap.cpp', u'Source/JavaScriptCore/heap/Heap.h', u'Source/JavaScriptCore/heap/Weak.h', u'Source/JavaScriptCore/heap/WeakBlock.cpp', u'Source/JavaScriptCore/heap/WeakImpl.h', u'Source/JavaScriptCore/heap/WeakInlines.h', u'Source/JavaScriptCore/heap/WeakSet.h', u'Source/JavaScriptCore/heap/WeakSetInlines.h', u'Source/JavaScriptCore/runtime/VM.h']" exit_code: 1
Source/JavaScriptCore/heap/WeakInlines.h:83:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/heap/WeakInlines.h:90:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/heap/Weak.h:64:  Missing spaces around &&  [whitespace/operators] [3]
Source/JavaScriptCore/heap/Weak.h:73:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 4 in 12 files


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

[Mark Hahnenberg]

Created attachment 213458 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSManagedValue.mm', u'Source/JavaScriptCore/API/tests/testapi.mm', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/heap/Heap.cpp', u'Source/JavaScriptCore/heap/Heap.h', u'Source/JavaScriptCore/heap/Weak.h', u'Source/JavaScriptCore/heap/WeakBlock.cpp', u'Source/JavaScriptCore/heap/WeakImpl.h', u'Source/JavaScriptCore/heap/WeakInlines.h', u'Source/JavaScriptCore/heap/WeakSet.h', u'Source/JavaScriptCore/heap/WeakSetInlines.h', u'Source/JavaScriptCore/runtime/VM.h']" exit_code: 1
Source/JavaScriptCore/heap/WeakInlines.h:90:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/heap/Weak.h:64:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 2 in 12 files


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

[Mark Hahnenberg]

Created attachment 213629 [details]
Patch

[Mark Hahnenberg]

Created attachment 213633 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 213633 [details]
Patch

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

> Source/JavaScriptCore/API/tests/testapi.mm:363
> +        checkResult(@"is_ham is nil", ![myPrivateProperties valueForKey:@"is_ham"]);
> +        checkResult(@"message is nil", ![myPrivateProperties valueForKey:@"message"]);

Need to update the test to check that the other entries are also nil.

[Mark Hahnenberg]

Committed r157119: <http://trac.webkit.org/changeset/157119>