289263 - document.all.tags no longer working

Status: RESOLVED FIXED

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

Description

[Robin Lu]

bug 248549 implemented document.all.tags. But after patch for bug 256640 was
checked  in, it does not work anymore.
I found there's some difference between the patch posted on bug 256640 and the
actual one being checked in. The patch posted on the bug does not break the
function. The affected part is:
@@ -5713,20 +5875,29 @@ nsHTMLDocumentSH::CallToGetPropMapper(JS
     return JS_FALSE;
   }
 
   // Convert all types to string.
   JSString *str = ::JS_ValueToString(cx, argv[0]);
   if (!str) {
     return JS_FALSE;
   }
 
-  return ::JS_GetUCProperty(cx, JSVAL_TO_OBJECT(argv[-2]),
-                            ::JS_GetStringChars(str),
+  JSObject *self = JSVAL_TO_OBJECT(argv[-2]);
+
+  if (::JS_ObjectIsFunction(cx, self)) {
+    // If self (aka this) is a function object we're called through
+    // document.all.item(), or something similar. In such a case, the
+    // this object is the parent of argv[-2].
+
+    self = ::JS_GetParent(cx, self);
+  }
+
+  return ::JS_GetUCProperty(cx, self, ::JS_GetStringChars(str),
                             ::JS_GetStringLength(str), rval);
 }
 
 
 static inline JSObject *
 GetDocumentAllHelper(JSContext *cx, JSObject *obj)
 {
   while (obj && JS_GET_CLASS(cx, obj) != &sHTMLDocumentAllHelperClass) {
     obj = ::JS_GetPrototype(cx, obj);

Comment 1

[Boris Zbarsky [:bzbarsky]]

And we landed that on the branches too... fun.

In any case, the problem is that document.all.tags() is indeed something that
looks like a function call, and it's _not_ happening on an object of class
sHTMLDocumentAllClass.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Or perhaps the problem is that it actually is happening on an
sHTMLDocumentAllClass but we want to be taking the other branch (the argv[-2] one)?

Comment 3

[Johnny Stenback (:jst)]

Anyone got a testcase for this laying around?

Comment 4

[Steve Chapel]

I wrote a testcase for bug 253408.

Comment 5

[Boris Zbarsky [:bzbarsky]]

On that testcase, the |str| passed to nsHTMLDocumentSH::CallToGetPropMapper is
"p", and |obj| is of class sHTMLDocumentAllClass.  We want to be ending up in
the argv[-2] branch, and we aren't.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Fix broken logic

This fixes the incorrect logic where we determine if 'this' is passed as
argv[-2] or as obj. If argv[-2] is a function, use obj, if not, use argv[-2].
With this patch the following cases work:

  document.all.foo (=== .all['foo'])
  document.all('foo')
  document.all.item('foo')
  document.all.tags.p (=== .tags['p'])
  document.all.tags('p')

and I think that's all we support.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 185082 [details] [diff] [review]
Fix broken logic

sr+a=me for 1.8b3.

/be

Comment 8

[Brendan Eich [:brendan]]

Technutz, please don't make the Summary ungrammatical again.  The subject of
that sentence is singular even though it ends in .tags.

/be

Comment 9

[Johnny Stenback (:jst)]

Fixed on the trunk.

Comment 10

[Robert Accettura [:raccettura]]

Is this by any chance what caused some blogger problems:
http://reporter.mozilla.org/app/query/?host_hostname=www.blogger.com&submit_query=true

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 185082 [details] [diff] [review]
Fix broken logic

1.0.5 and 1.7.9 have already shipped; unsetting approval requests.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185082 [details] [diff] [review]
Fix broken logic

Asa, we still need to take this on branch, since this is still broken there. 
Given that we've managed to ship the release this really should have been fixed
in without even looking at the approval flags, perhaps we should approve it for
the _next_ release, eh?

Comment 13

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

At this point we're better off just fixing this in 1.5.  No point tweaking our
API for Web developers yet again.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 185082 [details] [diff] [review]
Fix broken logic

branch approval denied per comment 13