Bug 96034 - Add new JSDependentRetained that allows keeping a JSObject alive as long as another is alive
Summary: Add new JSDependentRetained that allows keeping a JSObject alive as long as a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks: 93661
  Show dependency treegraph
 
Reported: 2012-09-06 15:57 PDT by Elliott Sprehn
Modified: 2012-09-11 17:40 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.47 KB, patch)
2012-09-06 16:00 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (13.06 KB, patch)
2012-09-06 17:09 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (13.08 KB, patch)
2012-09-06 17:20 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (13.12 KB, patch)
2012-09-06 17:25 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (14.07 KB, patch)
2012-09-11 14:37 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (14.07 KB, patch)
2012-09-11 15:46 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-09-06 15:57:30 PDT
Add new JSDependentRetained that allows keeping a JSObject alive as long as another is alive
Comment 1 Elliott Sprehn 2012-09-06 16:00:43 PDT
Created attachment 162609 [details]
Patch
Comment 2 Elliott Sprehn 2012-09-06 16:06:26 PDT
This is the JSC side of https://bugs.webkit.org/show_bug.cgi?id=95519
Comment 3 Geoffrey Garen 2012-09-06 16:40:41 PDT
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.
Comment 4 Geoffrey Garen 2012-09-06 16:41:24 PDT
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.
Comment 5 Elliott Sprehn 2012-09-06 17:08:40 PDT
(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).
Comment 6 Elliott Sprehn 2012-09-06 17:09:24 PDT
Created attachment 162621 [details]
Patch for landing
Comment 7 Elliott Sprehn 2012-09-06 17:20:35 PDT
Created attachment 162624 [details]
Patch for landing
Comment 8 Elliott Sprehn 2012-09-06 17:21:16 PDT
(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.
Comment 9 Elliott Sprehn 2012-09-06 17:25:38 PDT
Created attachment 162625 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-09-06 21:49:17 PDT
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
Comment 11 Elliott Sprehn 2012-09-06 21:55:16 PDT
(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?
Comment 12 Alexey Proskuryakov 2012-09-07 10:18:02 PDT
> 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.
Comment 13 Adam Barth 2012-09-07 10:22:01 PDT
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.
Comment 14 Elliott Sprehn 2012-09-11 14:37:22 PDT
Created attachment 163441 [details]
Patch for landing
Comment 15 Build Bot 2012-09-11 15:22:13 PDT
Comment on attachment 163441 [details]
Patch for landing

Attachment 163441 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13821483
Comment 16 Elliott Sprehn 2012-09-11 15:46:45 PDT
Created attachment 163463 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-09-11 17:40:43 PDT
Comment on attachment 163463 [details]
Patch for landing

Clearing flags on attachment: 163463

Committed r128249: <http://trac.webkit.org/changeset/128249>
Comment 18 WebKit Review Bot 2012-09-11 17:40:47 PDT
All reviewed patches have been landed.  Closing bug.