Closed Bug 484764 Opened 11 years ago Closed 11 years ago

Set up DOM prototype chain from nsDOMClassInfo::PostCreateProto

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(3 files, 1 obsolete file)

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).
Attached patch Preparation v1Splinter Review
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.
Attachment #368873 - Flags: superreview?(jst)
Attachment #368873 - Flags: review?(jst)
Attachment #368873 - Flags: superreview?(jst)
Attachment #368873 - Flags: superreview+
Attachment #368873 - Flags: review?(jst)
Attachment #368873 - Flags: review+
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
The leak failure is being tracked in bug 485355, still not clear if this was definitely the cause or not.
Attached patch v1 (obsolete) — Splinter Review
Attached patch v1.1Splinter Review
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.
Attachment #376334 - Attachment is obsolete: true
Attachment #380504 - Flags: superreview?(jst)
Attachment #380504 - Flags: review?(jst)
Attachment #380504 - Flags: superreview?(jst)
Attachment #380504 - Flags: superreview+
Attachment #380504 - Flags: review?(jst)
Attachment #380504 - Flags: review+
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.
(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.
http://hg.mozilla.org/mozilla-central/rev/a0a62342aacb
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 499777
Depends on: 500025
Depends on: 533401
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.