484764 - Set up DOM prototype chain from nsDOMClassInfo::PostCreateProto

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Setting up the DOM prototype chain from nsDOMClassInfo::PostCreateProto instead of from nsDOMClassInfo::PostCreate makes more sense, we won't try to do it every time a wrapper is created (just the first time a proto is created in a scope). We'll also need this for slim wrappers (since we won't get a PostCreate call for those).

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Preparation v1

Couple of changes, but there should be no functional change from this patch:
- removed some unused statics
- made some statics useable when not in a nsDOMClassInfo member function
- moved the class name/proto name check from PostCreate to PostCreateProto
- extracted code to convert from eTypeExternalClassInfoCreator to eTypeExternalClassInfo into a separate function (GetExternalClassInfo)
- extracted some code from nsWindowSH::GlobalResolve into a separate function (ResolvePrototype)
- remove unused flags argument from nsWindowSH::GlobalResolve

I'm also going to attach a diff -w of the code in ResolvePrototype with the original code in nsWindowSH::GlobalResolve.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: diff -w between nsWindowSH::GlobalResolve and ResolvePrototype

Comment 3

[Dave Townsend [:mossop]]

I've backed this out as it seems likely to have caused assertions on one slave for the leak tests:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238078157.1238078632.6826.gz
###!!! ASSERTION: Event listener manager hash not empty at shutdown!: 'sEventListenerManagersHash.entryCount == 0', file e:/builds/moz2_slave/mozilla-central-win32-debug/build/content/base/src/nsContentUtils.cpp, line 909

Comment 4

[Dave Townsend [:mossop]]

The leak failure is being tracked in bug 485355, still not clear if this was definitely the cause or not.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.1

I think these are the relevant changes:

- Remove PostCreate hooks for most DOM classes
- Add a DEBUG-only PostCreate hook for windows and move the code that checks for single wrapper in there
- Set up the prototype chain from PostCreatePrototype
- Store a pointer to the UTF-16 string from the nsScriptNameSpaceManager's hash key in the DOMCI data and use that pointer when setting up constructors (instead of looking in the hash every time just to get the key)
- Force creation of XPConnect prototype from GlobalResolve for DOM constructors so that PostCreatePrototype is called and we set up the prototype chain
- Have an empty PostCreatePrototype hook for nsDOMConstructorSH, there's no prototype chain to set up for DOMConstructor/DOMPrototype
- Make sure that XPCWrappedNativeProto::Init nulls out the links between XPCWrappedNativeProto and its JS object if PostCreatePrototype fails (which I triggered with an error in an earlier version of the patch)

Passes the unit tests on try server.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 380504 [details] [diff] [review]
v1.1

- In nsDOMConstructor::Install():

       ::JS_DefineUCProperty(cx, target,
                             reinterpret_cast<const jschar *>(mClassName),
                             nsCRT::strlen(mClassName), thisAsVal, nsnull,
-                            nsnull, 0);
+                            nsnull, JSPROP_PERMANENT);

I don't think I'm opposed to this change, but I'm curious as to why this is important for this bug?
 
- In GetXPCProto():

+    // In most cases we want to find the wrapped native prototype in
+    // aWin's scope and use that prototype for
+    // ClassName.prototype. But in the case where we're setting up
+    // "Window.prototype" or "ChromeWindow.prototype" we want to do
+    // the look up in aWin's outer window's scope since the inner
+    // window's wrapped native prototype comes from the outer
+    // window's scope.
+    if (ci_id == eDOMClassInfo_Window_id ||
+        ci_id == eDOMClassInfo_ChromeWindow_id) {

Seems like eDOMClassInfo_ModalContentWindow_id should be in that list (and in the comment) too. Looks like a bug in code you moved around here, not something you added...

- In nsWindowSH::GlobalResolve():

+    if (alias_struct->mType == nsGlobalNameStruct::eTypeClassConstructor) {
+      ci_data = &sClassInfoData[alias_struct->mDOMClassInfoID];
+    } else if (alias_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo) {
+      ci_data = alias_struct->mData;
+    }
+    else {

Pull the else up after the closing brace.

- In nsDOMClassInfo.h:

+#ifdef DEBUG
...
+  NS_IMETHOD GetScriptableFlags(PRUint32 *aFlags)
+  {
+    PRUint32 flags;
+    nsresult rv = nsEventReceiverSH::GetFlags(&flags);

This looks really wrong. You mean nsEventReceiverSH::GetScriptableFlags() here, right?

r+sr=jst with that looked into.

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to comment #7)
> - In nsDOMConstructor::Install():
> 
>        ::JS_DefineUCProperty(cx, target,
>                              reinterpret_cast<const jschar *>(mClassName),
>                              nsCRT::strlen(mClassName), thisAsVal, nsnull,
> -                            nsnull, 0);
> +                            nsnull, JSPROP_PERMANENT);
> 
> I don't think I'm opposed to this change, but I'm curious as to why this is
> important for this bug?

I think I discussed this with mrbkap. We used to do a lookup by name on the window to find a DOM constructor to put on the prototype chain but after this patch we just grab the right DOM constructor directly when the XPC proto is created. Deleting the DOM constructor wouldn't change what we put on the prototype chain, this change tries to make that more explicit. I guess we could also make this JSPROP_READONLY, not sure if that would break anything.

Comment 9

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/a0a62342aacb