Bug 36420 - JSC needs an API to allow custom objects to have aprivate GC-accessible properties
Summary: JSC needs an API to allow custom objects to have aprivate GC-accessible prope...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-20 16:26 PDT by Oliver Hunt
Modified: 2010-03-21 13:19 PDT (History)
2 users (show)

See Also:


Attachments
Patch (18.88 KB, patch)
2010-03-20 16:31 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (20.02 KB, patch)
2010-03-20 20:25 PDT, Oliver Hunt
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2010-03-20 16:26:36 PDT
JSC needs an API to allow custom objects to have aprivate GC-accessible properties
Comment 1 Oliver Hunt 2010-03-20 16:31:36 PDT
Created attachment 51238 [details]
Patch
Comment 2 Maciej Stachowiak 2010-03-20 18:57:55 PDT
Comment on attachment 51238 [details]
Patch

Comments given on IRC:

Docs:

the headerdoc could maybe explain the purpose of this
i.e. that it's a way to hold on to JavaScript values in a way that is safe for GC
I think probably in the set one would be the best place to explain the purpose
also maybe add a note to JSValueProtect
to make clear that if you want one JS object to keep another alive, you shouldn't use it, you should use a private property
otherwise you may get uncollectable reference cycles


Error cases:

JSObjectSetPrivateProperty seems to fail silently if the argument is not a callback object
maybe it should have a boolean return? or raise an exception?
it looks like JSObjectGetPrivateProperty will return garbage when called on a non-API object
I think it would make more sense to return undefined
Silent failure on delete seems ok

Testing:

you could try harder in the test case to avoid the possibility that a copy of aHeapRef remains on the stack
I think you should test calling these on non-API objects in the test case
Comment 3 Oliver Hunt 2010-03-20 20:19:08 PDT
(In reply to comment #2)
> (From update of attachment 51238 [details])
> Comments given on IRC:
> 
> Docs:
> 
> the headerdoc could maybe explain the purpose of this
> i.e. that it's a way to hold on to JavaScript values in a way that is safe for
> GC
> I think probably in the set one would be the best place to explain the purpose
> also maybe add a note to JSValueProtect
> to make clear that if you want one JS object to keep another alive, you
> shouldn't use it, you should use a private property
> otherwise you may get uncollectable reference cycles
I'm not sure about adding references to the new API from actual API until the new API has gone through review.

> 
> 
> Error cases:
> 
> JSObjectSetPrivateProperty seems to fail silently if the argument is not a
> callback object
> maybe it should have a boolean return? or raise an exception?

i'll make it return bool as setPrivate does

> it looks like JSObjectGetPrivateProperty will return garbage when called on a
> non-API object
> I think it would make more sense to return undefined

It returns NULL

> Silent failure on delete seems ok
I'll return bool to be consistent with SetPrivateProperty


> 
> Testing:
> 
> you could try harder in the test case to avoid the possibility that a copy of
> aHeapRef remains on the stack

It does get blown from the stack -- i verified the test failed before i added marking support :D

> I think you should test calling these on non-API objects in the test case

Done.
Comment 4 Oliver Hunt 2010-03-20 20:25:45 PDT
Created attachment 51240 [details]
Patch
Comment 5 Maciej Stachowiak 2010-03-20 23:57:47 PDT
Comment on attachment 51240 [details]
Patch

Please add @result to the headerdoc for JSObjectSetPrivateProperty. Otherwise r=me.
Comment 6 Oliver Hunt 2010-03-21 00:40:20 PDT
Committed r56314: <http://trac.webkit.org/changeset/56314>
Comment 7 Eric Seidel (no email) 2010-03-21 05:11:51 PDT
This appears to have broken the windows build.
Comment 8 David Kilzer (:ddkilzer) 2010-03-21 13:19:27 PDT
(In reply to comment #7)
> This appears to have broken the windows build.

Blind attempt #1 to fix the Windows build (failed):
<http://trac.webkit.org/changeset/56317>

Blind attempt #2 (worked):
<http://trac.webkit.org/changeset/56318>