50080 - document.links[0] gives 'can't convert link to primitive type'

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

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

Found while investigating bug 49941. jst said brendan did some changes to lazily 
evaluate stuff so assigning this to brendan first. I will attach a test case.

Comment 1

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

Attachment: Testcase

Comment 2

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

In the testcase, if you mouse over a link you would expect an alert box saying:

document.links[0]=[object HTMLAnchorObject]

Instead you get JS error (in console):

JavaScript error:
 line 0: can't convert document.links[0] to primitive type

If you change the test case slightly and access document.links[0].href that 
works fine.

Comment 3

[Brendan Eich [:brendan]]

I only changed the standard JS classes (String, etc.) and their top-level
properties (isNaN, e.g.) to be lazily initialized, not any DOM classes.  So I
don't think this is mine.  Is anyone able to debug it a bit?  I'll look at it
when I get a chance, not right now.

/be

Comment 4

[Brendan Eich [:brendan]]

BTW, 4.x and earlier (I wrote the original code in September, 1995) give URL
objects a toString method that returns the href property's value:

http://lxr.mozilla.org/classic/source/lib/libmocha/lm_url.c#426

Does some DOM spec say the result of toString for an <a href=...>...</a> tag
should be "[object HTMLAnchorObject]"?  In any case, should Mozilla really be
incompatible?

/be

Comment 5

[John Bandhauer]

Brendan,

I debugged some and have more data (which I don't fully understand).

I added a case for comparision that dumps 'window'. For 'window' we, of course, 
call the global resolve. We get to the point where we are in the loop in  
js_LookupProperty looking to 'toString'. The target object itself has no 'sym'. 
It's proto does. We succeed and go on our way.

For the link case... the resolver called is the generic resolve which does 
nothing interesting except return JS_TRUE. In js_LookupProperty we look for 
'sym' using the main lookup at the top (scope->object == obj is true all the way 
up the proto chain). We eventually get to the Node class which has no proto and 
we return JS_TRUE with the out params set to null. So js_GetProperty ends up 
calling js_GetSlotWhileLocked and we end up with a result of JSVAL_VOID and we 
have no toString method to call.

So what was supposed to happen?

One of those calls to "scope->ops->lookup" was supposed to give us a sym, no?

Comment 6

[John Bandhauer]

I haven't debugged more and don't understand this any better yet. But, I can't 
help thinking that the problem has to do with the core std classes 
(just Object & Function?) not being init'd yet at the time that the Node (etc.) 
classes get init'd.

Comment 7

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

I did not check what DOM says what we should report on this case, but in general 
that is at least what Mozilla does with all elements. 

As to if we should mimic NN 4.x (covered in bug 49941 which I marked WONTFIX) I 
say no. There is a simple workaround for NN 4.x and that is to use 
document.links[0].href which is correct and works both in Mozilla and NN 4.x.

Comment 8

[John Bandhauer]

Attachment: proposed fix

Comment 9

[John Bandhauer]

Attached a proposed fix. 

Brendan and I debugged and he suggested that the problem is the early returns in 
nsJSUtils::nsGlobalResolve that keep nsGenericResolve from getting called during 
the InitClass calls. The scriptContext stuff is only needed for the 
NameSpaceManager. Early on we might not have the scriptContext (or 
IsContextInitialized might not succeed) but that does not mean we shouldn't do 
the call to nsGenericResolve.

The patch fixes the problem. I don't see any bad side effect - but I have not 
done exhaustive DOM testing or anything.

Also, should we assert if GetNameSpaceManager fails?

reviews? comments?

Comment 10

[John Bandhauer]

r=? jst? vidur? heikki? anyone?

Comment 11

[Brendan Eich [:brendan]]

I wondered what happened to this bug!  Please cc: me as [e-mail].  I 
want your fix in, jband -- just a few nits here:

+          if (isConstructor) {
+            JS_DefineFunction(aContext, aObj, 
+                              JS_GetStringBytes(jsstring),
+                              StubConstructor, 0,
+                              JSPROP_READONLY);
+            return JS_TRUE;

Better to return JS_DefineFunction(...), which could fail.

+          }
+          else {

No else here, it's a non-sequitur after return.

+    
+            return JS_DefineProperty(aContext, aObj, 
JS_GetStringBytes(jsstring),
+                                     val, nsnull, nsnull, 
+                                     JSPROP_ENUMERATE | JSPROP_READONLY);
+          }

Should use JS_DefineUCProperty here, passing JS_GetStringChars(jsstring) (ulp, I 
hope I can constipate that return type at this late date!), in case someone 
wants to support resolving a Unicode identifer (legit in ECMA ed. 3).  This is 
not so important if the ScriptNameSet stuff doesn't support Unicode, but it's 
worth doing now because it reduces bloat in JS's deflated string cache.

Fix these, and a=[e-mail].

/be

Comment 12

[John Bandhauer]

Sorry about the '[e-mail]' - I know better.

Changes with nits fixed checked in.

Comment 13

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

Tested on NT and Linux, seems to work so marking verified.