Bug 96034 – Add new JSDependentRetained that allows keeping a JSObject alive as long as another is alive

• Component: New Bugs
• Status: RESOLVED FIXED

[Elliott Sprehn]

Add new JSDependentRetained that allows keeping a JSObject alive as long as another is alive

[Elliott Sprehn]

Created attachment 162609 [details]
Patch

[Elliott Sprehn]

This is the JSC side of https://bugs.webkit.org/show_bug.cgi?id=95519

[Geoffrey Garen]

Comment on attachment 162609 [details]
Patch

Adding and deleting a property will have the negative side-effect of making the node wrappers uncollectable until the nodes are removed from the document, and of allocating extra structures, one per node wrapper.

If this is common, another possible solution is for JSDependentRetained to implement the WeakHandleOwner interface, save "owner" as its context, and implement "isReachableFromOpaqueRoots" to test whether "owner" has been marked. This will achieve the same lifetime semantics without the other costs.

[Geoffrey Garen]

I believe the preferred license for new files is http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE. Please use that, unless you have a specific reason not to.

[Elliott Sprehn]

(In reply to comment #3)
> (From update of attachment 162609 [details])
> Adding and deleting a property will have the negative side-effect of making the node wrappers uncollectable until the nodes are removed from the document, and of allocating extra structures, one per node wrapper.
> 
> If this is common, another possible solution is for JSDependentRetained to implement the WeakHandleOwner interface, save "owner" as its context, and implement "isReachableFromOpaqueRoots" to test whether "owner" has been marked. This will achieve the same lifetime semantics without the other costs.

Okay. This is for rare things like MutationObservers and UndoManager. If we decide to use it in the future for common things I'll make that change (as we'll also need to update V8DependentRetained to use v8::AddImplicitReference instead of hidden props).

[Elliott Sprehn]

Created attachment 162621 [details]
Patch for landing

[Elliott Sprehn]

Created attachment 162624 [details]
Patch for landing

[Elliott Sprehn]

(In reply to comment #7)
> Created an attachment (id=162624) [details]
> Patch for landing

Sorry, I realized the boolean cast of Handle was wrong, you need to do IsEmpty() on it.

[Elliott Sprehn]

Created attachment 162625 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162625 [details]
Patch for landing

Rejecting attachment 162625 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
547 (offset 2 lines).
Hunk #2 succeeded at 13976 (offset 4 lines).
Hunk #3 succeeded at 20169 (offset 6 lines).
Hunk #4 FAILED at 25503.
1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/WebCore.xcodeproj/project.pbxproj.rej
patching file Source/WebCore/bindings/js/JSDependentRetained.h
patching file Source/WebCore/bindings/v8/V8DependentRetained.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13765884

[Elliott Sprehn]

(In reply to comment #10)
> (From update of attachment 162625 [details])
> ...
> Last 500 characters of output:
> 547 (offset 2 lines).
> Hunk #2 succeeded at 13976 (offset 4 lines).
> Hunk #3 succeeded at 20169 (offset 6 lines).
> Hunk #4 FAILED at 25503.
> 1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/WebCore.xcodeproj/project.pbxproj.rej

I don't understand what happened here. Is this a merge conflict on the project file? Do I sync and upload again?

[Alexey Proskuryakov]

> I don't understand what happened here. Is this a merge conflict on the project file? Do I sync and upload again?

Yes. Please also wait for EWS to check non-chromium builds, as almost all of the bubbles are purple.

[Adam Barth]

Comment on attachment 162625 [details]
Patch for landing

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25506
> +				FFD86E7815F9583600047233 /* JSDependentRetained.h in Headers */,

The problem is that Xcode is dumb and puts new stuff at the end of these sorts of blocks, which means there are always conflicts.  I usually edit the xcodeproj by hand and move new lines to where they go alphabetically.  If you're feeling ambitious, you can run sort-xcodeproj to sort the entire file.

[Elliott Sprehn]

Created attachment 163441 [details]
Patch for landing

[Build Bot]

Comment on attachment 163441 [details]
Patch for landing

Attachment 163441 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13821483

[Elliott Sprehn]

Created attachment 163463 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 163463 [details]
Patch for landing

Clearing flags on attachment: 163463

Committed r128249: <http://trac.webkit.org/changeset/128249>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.