91557 - constructor property of DOM Object instances return incorrect constructor

Status: RESOLVED FIXED

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

Description

[tfriesen]

In recent builds (July 19),the constructor property of (most,if not all) DOM 
object instances  return the Object object.

For example:

document.body.constructor==HTMLBodyElement //returns false, should return true

Comment 1

[Johnny Stenback (:jst)]

Yup, this is a known problem, or rather a know missing feature that we haven't
had a chance to fix yet. Trying for mozilla0.9.4

Comment 2

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

Comment 3

[Johnny Stenback (:jst)]

jband, could you have a look at the attached patch, the patch implements
DOMObject.constructor by hooking any objects .constructor property to
window.classname, sr=?

Comment 4

[John Bandhauer]

sr=jband

It looks like it ought to do the right thing. You've tested of course.

If you care about distinguishing the case where val is null then you should use
  if (!JSVAL_IS_PRIMITIVE(val))
rather than
  if (JSVAL_IS_OBJECT(val))

Comment 5

[Johnny Stenback (:jst)]

Attachment: Fix including !JSVAL_IS_PRIMITIVE() changes suggested by jband

Comment 6

[harishd]

r=harishd

Comment 7

[Johnny Stenback (:jst)]

This won't make it onto the m094 branc, not even post mozilla094

Comment 8

[Johnny Stenback (:jst)]

Er, s/branc/bransh/

Anyway, the attached fix does fix the problem, but it causes typeof to break
miserably for DOM objects :-(

Comment 9

[Johnny Stenback (:jst)]

Pushing to mozilla0.9.7

Comment 10

[Johnny Stenback (:jst)]

Pushing to mozilla1.0

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

nsbeta1-

Before you renominate, please query bugs marked nsbeta1+ keyword with [ADT# RTM]
in status whiteboard (where # is a number between 1 and 3) and make sure that
this bug is at least as important as those.

Comment 12

[Peter Van der Beken [:peterv]]

Taking.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 14

[Johnny Stenback (:jst)]

#define NS_DOM_JS_CONSTRUCTOR(_class)                                         \
static JSBool JS_DLL_CALLBACK                                                 \
_class##_DOMJSNativeConstructor(JSContext *cx, JSObject *obj, uintN argc,     \
                                jsval *argv, jsval *rval)                     \
{                                                                             \
  *rval = JSVAL_VOID;                                                         \
  nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_DOM_NOT_SUPPORTED_ERR);       \
  NS_ERROR("object instantiated without constructor");                        \
  return JS_FALSE;                                                            \
}

This will end up defining a 19 instruction long function per class in
nsDOMClassInfo, if you change the above to:

static void
DOMJSNativeConstructorHelper(JSContext *cx, jsval *rval)
{
  *rval = JSVAL_VOID;
  nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_DOM_NOT_SUPPORTED_ERR);
  NS_ERROR("object instantiated without constructor");
}

#define NS_DOM_JS_CONSTRUCTOR(_class)                                         \
static JSBool JS_DLL_CALLBACK                                                 \
_class##_DOMJSNativeConstructor(JSContext *cx, JSObject *obj, uintN argc,     \
                                jsval *argv, jsval *rval)                     \
{                                                                             \
  DOMJSNativeConstructorHelper(cx, rval);                                     \
  return JS_FALSE;                                                            \
}

we'll cut down the size of these functions to 9 instructions (in stead of 19).
These numbers are from optimized code on linux, using gcc-2.96. I'd imagine
similar savings on all compilers.

Comment 15

[Peter Van der Beken [:peterv]]

Jst also noted that I should check the order in which we resolve the constructor
property against how ns4.x does it. For example with a frame named "constructor"
will window.constructor be that frame or the window object's constructor?

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 17

[Peter Van der Beken [:peterv]]

Jonas: want to review?

Comment 18

[Johnny Stenback (:jst)]

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

A few minor comments:

- In nsDOMClassInfo.cpp:

+static void
+DOMJSNativeConstructorHelper(JSContext *cx, jsval *rval)
+{

Maybe add a comment here to explain that this exists only to minimize the
constructor functions?

- In nsDOMClassInfo::NewResolve():

+  if ((id == sConstructor_id) &&
+      !(flags & JSRESOLVE_ASSIGNING)) {

This would fit on one line, move it...

-    } else {
-      native = NativeConstructor;
     }
-
+    else {

<sigh/> 'else' belongs on the same line as the closing bracket for the if blok
in this file! :-)

- In mozilla/dom/src/build/Makefile.in:

+# MSVC '-Gy' cc flag and '/OPT:REF' linker flag cause the DOM JS constructor
+# functions to be folded to the same address by the linker. So, in optimized
+# builds, add the /OPT:NOICF flag, which turns off 'identical COMDAT folding'.
+#
+# N.B.: 'identical COMDAT folding' that folds functions whose addresses
+#	 are taken violates the ISO C and C++ standards.
+ifndef MOZ_DEBUG
+LDFLAGS	 += /OPT:NOICF
+endif

I know we don't really have a choice here, so just out of curiosity, how big an
impact does this have on the size of jsdom.dll on Win32?

Other than that, sr=jst

Comment 19

[Brendan Eich [:brendan]]

^Ms in the patch, yuck.

+static void
+DOMJSNativeConstructorHelper(JSContext *cx, jsval *rval)
+{
+  *rval = JSVAL_VOID;

The JS engine doesn't need *rval to be set if you're going to fail/throw (return
false).  Does the DOM code?

+  nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_DOM_NOT_SUPPORTED_ERR);
+  NS_ERROR("object instantiated without constructor");
+}
+
+#define NS_DOM_JS_CONSTRUCTOR(_class)                                         \
+static JSBool JS_DLL_CALLBACK                                                 \
+_class##_DOMJSNativeConstructor(JSContext *cx, JSObject *obj, uintN argc,     \
+                                jsval *argv, jsval *rval)                     \
+{                                                                             \
+  DOMJSNativeConstructorHelper(cx, rval);                                     \
+  return JS_FALSE;                                                            \
+

Probably you'd save even more by moving the return JS_FALSE into
DOMJSNativeConstructorHelper, and returning the r.v. of that function in each
macro-expanded stub.  Similar idea for _class##DOMJSConstructor -- or can't you
call a static helper from all those NS_DOM_JS_CONSTRUCTOR_HELPER
macro-expansions, because some are in a different DLL/DSO?

Are DOM constructors rarely used (usually by instanceof usage)?  If so, you
should be fully lazy.  One idea is to roll your own function-like JSClass with
its own JSClass.hasInstance for these DOM constructor objects.  That may be more
bloated, though.  How much are the minimal stubs (with the above chained return
idea) costing, anyhoo?

/be

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: v1.2

jsdom.dll size from my optimized Windows build:

Before patch: 200,704 bytes
After patch: 204,800 bytes

So about 4k of bloat, not sure if this would warrant creating our own JSClass.

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 110949 [details] [diff] [review]
v1.2

I've started implementing Brendan's suggestions, and I like it better.

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 111709 [details] [diff] [review]
v2

- In nsDOMClassInfo::ResolveConstructor():

+  JSObject *global = obj;
+  JSObject *tmp;
+
+  while ((tmp = ::JS_GetParent(cx, global))) {
+    global = tmp;
+  }

Use global = GetGlobalObject(cx, obj) here.

- nsDOMClassInfo::NewResolve():

+  if ((id == sConstructor_id) && !(flags & JSRESOLVE_ASSIGNING)) {
+    return ResolveConstructor(cx, obj, objp);
+  }

Loose the extra parens.

- In DOMJSClass_construct():

+  PRUnichar *class_name = (PRUnichar *)::JS_GetPrivate(cx, class_obj);

I'd feel more comfortable with this if class_name was const.

+  gNameSpaceManager->LookupName(nsDependentString(class_name),
+				 &name_struct);
+  if (!name_struct) {
+    return JS_FALSE;
+  }

Can name_struct be null? If so, don't you want to throw an exception here?

+  if ((name_struct->mType == nsGlobalNameStruct::eTypeExternalClassInfo &&
+	name_struct->mData->mConstructorCID) ||
+      name_struct->mType == nsGlobalNameStruct::eTypeExternalConstructorAlias)
{
+    return BaseStubConstructor(name_struct, cx, obj, argc, argv, rval);
+  }

   // ignore return value, we return JS_FALSE anyway
   nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_DOM_NOT_SUPPORTED_ERR);

Flip this around, throw the exception and return early inside the if, and let
the normal program flow to the end of the function.

- In DOMJSClass_finalize():

+  PRUnichar *class_name = (PRUnichar *)::JS_GetPrivate(cx, obj);
+  nsMemory::Free(class_name);

What if !class_name, check before freeing.

- In DOMJSClass_hasinstance():

+  if (!::JS_ValueToObject(cx, v, &dom_obj)) {
+    return JS_FALSE;

Throw exception here too?

+  JSClass* dom_class = JS_GetClass(cx, dom_obj);

Use JS_GET_CLASS() here in stead of calling JS_GetClass() directly.

+  PRUnichar *class_name = (PRUnichar *)::JS_GetPrivate(cx, obj);

Make class_name const here too.

+  while (ci_data->mInterfaces[count]) {

You *could* add a local that you set to point to ci_data->mInterfaces[count++]
in the while condition to avoid multiple accesses to the interfaces array.
Either way is cool with me, your call...

+JSClass nsDOMClassInfo::sDOMJSClass = {
+  "DOM Class", JSCLASS_HAS_PRIVATE,
+  JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub,
+  JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, DOMJSClass_finalize,
+   nsnull, nsnull, nsnull, DOMJSClass_construct,
+   nsnull, DOMJSClass_hasinstance
+};

Wanna rename the hooks to DOMJSClass_Finalize, ..._Construct, and
..._HasInstance to make the names match the ones used in the JS engine better?

+    if (!::JS_SetPrivate(cx, class_obj, ToNewUnicode(name))) {
+      return NS_ERROR_UNEXPECTED;
+    }

What if ToNewUnicode() returns null? Return E_OOM. Same thing in one more
place.

@@ -3738,6 +3812,11 @@
     return NS_OK;
   }

+  if ((id == sConstructor_id) &&
+      !(flags & JSRESOLVE_ASSIGNING)) {

Doesn't this fit on one line? And remove the extra parens.

Other than that this looks fine with me, r/sr=jst, and Brendan gave sr=brendan
in private email.

Comment 24

[Peter Van der Beken [:peterv]]

> Use global = GetGlobalObject(cx, obj) here.

global = ::JS_GetGlobalObject(cx); or did you mean something else?

(This patch doesn't seem to affect library size on my windows optimized build)

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 112193 [details] [diff] [review]
v2.1

- In nsDOMClassInfo::ResolveConstructor():

+  JSObject *global = ::JS_GetGlobalObject(cx);

Sorry, I meant GetGlobalJSObject(), not GetGlobalObject(), and really not
JS_GetGlobalObject() :-) There's a static inline GetGlobalJSObject() in
nsDOMClassInfo.cpp that does exactly what your original code did, use that.

+    nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_UNEXPECTED);
+    NS_ERROR("...");

I see this pattern alot in this code, in old code as well as in new code. I'd
prefere to see this flipped around, that way you can step over the throwing
code in the debugger if you ever hit one of those asserts. Not a big deal,
change it if you agree and have the time to make that change.

+JSClass nsDOMClassInfo::sDOMJSClass = {
+  "DOM Class", JSCLASS_HAS_PRIVATE,
+  JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub,
+  JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, DOMJSClass_Finalize,
+   nsnull, nsnull, nsnull, DOMJSClass_Construct,
+   nsnull, DOMJSClass_HasInstance

3-space indentation? :-)

+JSFunctionSpec nsDOMClassInfo::sDOMJSClass_methods[] = {
+  {"toString", DOMJSClass_toString, 0, 0, 0},
+  {0,0,0,0,0}
+};

Inconsistent space-after-comma usage.

+    const PRUnichar* class_name = ToNewUnicode(name);
+    NS_ENSURE_TRUE(class_name, NS_ERROR_OUT_OF_MEMORY);

-    if (!cfnc_obj) {
+    if (!::JS_SetPrivate(cx, class_obj, ToNewUnicode(name))) {

Change ToNewUnicode() in the call to ::JS_SetPrivate() to class_name, or you'll
leak class_name...

sr=jst

Comment 27

[Peter Van der Beken [:peterv]]

Checked in.

Comment 28

[Peter Van der Beken [:peterv]]

This caused bug 190064, so I backed out the part that uses JS_DefineObject to
define classes not on a DOM proto chain. I obviously need to call
DefineInterfaceConstants on the object for those classes, but that doesn't work
and I don't understand why. The easy way to verify it is
|KeyEvent.DOM_VK_CANCEL|, which always returns 0.

Comment 29

[Peter Van der Beken [:peterv]]

Attachment: v2.2

This restores the part I backed out, adds the missing call to
DefineInterfaceConstants and fixes the endianess problem with
DefineInterfaceConstants that caused it to fail on Mac for (U)INT32 constants.
It now also reuses the name string in the script namespace manager's hash for
the JS objects.

Comment 30

[Peter Van der Beken [:peterv]]

Attachment: v2.3

Comment 31

[Peter Van der Beken [:peterv]]

Attachment: v2.4

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 112798 [details] [diff] [review]
v2.4

sr=jst

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 112798 [details] [diff] [review]
v2.4

r=sicking

Comment 34

[Peter Van der Beken [:peterv]]

Comment on attachment 112798 [details] [diff] [review]
v2.4

This restores the part of the original patch that I had to back out because of
a smoketest blocker. It now contains a fix for the smoketest blocker, also
reduces memory usage in the DOM that the original patch added and fixes a
potential issue where some DOM constants were broken on Mac. Low to medium
risk, but I'm fairly confident that it won't cause any regressions.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 112798 [details] [diff] [review]
v2.4

This seems like it could be a bit risky for this stage in 1.3b, and we have
enough problems already.  Could this wait for 1.4alpha?

Comment 36

[Peter Van der Beken [:peterv]]

Comment on attachment 112798 [details] [diff] [review]
v2.4

Please reconsider. The current situation causes the DOM objects to be
inconsistent. Objects on a proto chain (like HTMLDocument, Document, etc)
already use the new code while objects not on the proto chain (like
EventTarget, Event, etc) don't. This patch would fix that. I haven't seen any
reports of regressions for objects on a proto chain in the last week.

Comment 37

[Peter Van der Beken [:peterv]]

Attachment: v2.4 (diff -uw)

Attaching diff -uw version.

Comment 38

[Johnny Stenback (:jst)]

I agree with peterv here, please reconsider, the more testing we get on this the
better, and so far we've seen no problems from the code that this would enable
for a few more classes.

Comment 39

[Asa Dotzler [:asa]]

Comment on attachment 112798 [details] [diff] [review]
v2.4

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 40

[Peter Van der Beken [:peterv]]

Checked in.