Bug 36777 - [V8] Generate creation of hidden window references for DOMWindow attributes
Summary: [V8] Generate creation of hidden window references for DOMWindow attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-29 13:23 PDT by Nate Chapin
Modified: 2010-04-02 13:23 PDT (History)
2 users (show)

See Also:


Attachments
patch (24.68 KB, patch)
2010-03-29 13:34 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
add a more general solution for keeping js wrappers alive through gc (31.35 KB, patch)
2010-03-31 13:28 PDT, Nate Chapin
dglazkov: review+
Details | Formatted Diff | Diff
Add a couple more exceptions (31.00 KB, patch)
2010-04-01 16:20 PDT, Nate Chapin
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-03-29 13:23:55 PDT
(This bug began life as http://code.google.com/p/chromium/issues/detail?id=38990)

In the V8 bindings, we have a series of custom toV8() implementations for objects that need to ensure that their JS wrappers outlive garbage collection, so a reference is create to them on the JS wrapper of the relevant DOMWindow. There are two problems with this:
1. Custom toV8() implementation are teh suck.
2. It requires more than a passing knowledge of the v8 bindings to know that a custom toV8() implementation is necessary for a new object with this requirement (again, see chromium bug).

I believe that the case where a hidden reference is necessary is an (1) attribute of (2) DOMWindow that (3) is non-node, (4) non-EventListener, (5) non-DOMWindow, (6) non-Constructor, (7) and RefCounted (the last requirement filters out attributes that are int/double/etc).

My patch will do the following:
1. Change how we create the hidden references on the DOMWindow wrapper so that we don't need to manually specify an internal field for each variable that needs a hidden reference.
2. Move the logic of creating the hidden reference from the toV8() implementation to the generated attribute getter.
3. Delete a bunch of custom toV8() implementations.
4. Add more precision in the COMPILE_ASSERTs in V8DOMWrapper.cpp, since there is code that depends on the number of internal fields on a DOMWindow JS wrapper, and that number will no longer be nearly as unique as it once was.
Comment 1 Nate Chapin 2010-03-29 13:34:04 PDT
Created attachment 51959 [details]
patch
Comment 2 Michael Nordman 2010-03-29 13:43:07 PDT
This looks like it will fix the bug i was seeing around the applicationCache wrapper going 'poof'... and for all other DOMWindow object attributes.

I'm mildly concerned there are other instances of this problem with object attributes hanging off of things besides DOMWindow. For example,
XmlHttpRequest and its upload attribute, on which event handlers are hung.

So I'm wondering if a more general solution that works for DOMWindow and XmlHttpRequest alike is possible?
Comment 3 Nate Chapin 2010-03-31 13:28:55 PDT
Created attachment 52207 [details]
add a more general solution for keeping js wrappers alive through gc

The solution now deals with wrappers for non-Node DOM objects of a wide variety of types, not just ones that hang off of the DOMWindow.

I'm getting builds ready to run performance tests, don't know for sure whether this will have an impact.
Comment 4 Michael Nordman 2010-03-31 15:26:36 PDT
Excellent!
Comment 5 Dimitri Glazkov (Google) 2010-04-01 08:43:09 PDT
Comment on attachment 52207 [details]
add a more general solution for keeping js wrappers alive through gc

Looks good.
Comment 6 Nate Chapin 2010-04-01 16:20:01 PDT
Created attachment 52349 [details]
Add a couple more exceptions

Don't set a hidden reference is the current interface is a Node subtype (for performance).

Skip attributes named self (fixes some WorkerContext handling issues)
Comment 7 Dimitri Glazkov (Google) 2010-04-02 09:32:58 PDT
Comment on attachment 52349 [details]
Add a couple more exceptions

even more r=me.