Last Comment Bug 626050 - document.all() doesn't work, even in quirks mode
: document.all() doesn't work, even in quirks mode
Status: RESOLVED FIXED
fixed-in-tracemonkey, hardblocker
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Andreas Gal :gal
:
Mentors:
Depends on:
Blocks: 547046 747617
  Show dependency treegraph
 
Reported: 2011-01-15 05:56 PST by Siddharth Agarwal [:sid0] (inactive)
Modified: 2012-04-21 01:18 PDT (History)
17 users (show)
Ms2ger: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
test case (169 bytes, text/html)
2011-01-15 05:56 PST, Siddharth Agarwal [:sid0] (inactive)
no flags Details
patch (1.29 KB, patch)
2011-01-16 16:38 PST, Andreas Gal :gal
brendan: review+
jst: review+
Details | Diff | Review

Description Siddharth Agarwal [:sid0] (inactive) 2011-01-15 05:56:27 PST
Created attachment 504119 [details]
test case

(document.all is ancient and non-standard, but one of the intranet apps here depends on it. I tried looking for bugs on this but couldn't find any.)

The attached test case doesn't work in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110114 Firefox/4.0b10pre, but works fine in both IE9 and Chrome 8.0.552.237.

The expected output is [object HTMLDivElement];[object HTMLDivElement]. The output as seen in Minefield is [object HTMLDivElement];undefined.
Comment 1 Siddharth Agarwal [:sid0] (inactive) 2011-01-15 05:57:31 PST
I guess this isn't really worth blocking for, but there's no harm asking.
Comment 2 Olli Pettay [:smaug] 2011-01-15 06:00:20 PST
Is this a regression? If so, it would be great to have regression range.
Comment 3 Siddharth Agarwal [:sid0] (inactive) 2011-01-15 06:01:12 PST
Yes, it's a regression. It works in Firefox 3.6.13. I guess I should start bisecting...
Comment 4 Olli Pettay [:smaug] 2011-01-15 06:14:06 PST
I thought .all used [] not (), but apparently both need to be supported.
Comment 5 Siddharth Agarwal [:sid0] (inactive) 2011-01-15 06:25:42 PST
m-c regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ddfecbc93934&tochange=a29d44f196a6

Probably one of the TM changesets.
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2011-01-15 06:33:16 PST
TM regression range:
https://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=110112bebb00&tochange=dca026286095

(works in 2010-02-23, broken in 2010-02-25)

Bug 547046, maybe?
Comment 7 Siddharth Agarwal [:sid0] (inactive) 2011-01-15 06:34:07 PST
(In reply to comment #6)
> 
> (works in 2010-02-23, broken in 2010-02-25)

Oops, sorry. Works in 2010-02-22, broken in 2010-02-23.

m-c works in 2010-02-24, broken in 2010-02-25.
Comment 8 Jonas Sicking (:sicking) 2011-01-15 08:55:56 PST
over to js folks then. This is a commonly used API (though I'm not 100% sure how common the () syntax is), so recommending [hardblocker]
Comment 9 Brendan Eich [:brendan] 2011-01-15 10:44:36 PST
(In reply to comment #8)
> over to js folks then. This is a commonly used API (though I'm not 100% sure
> how common the () syntax is), so recommending [hardblocker]

This broke a while ago and it was just reported, so it is not a hardblocker in my view -- too much of a corner case of IE bogo-feature emulation. We can fix it in a .x release if the patch is safe. But we should see what exactly regressed this, in case it broke other things.

/be
Comment 10 Andreas Gal :gal 2011-01-15 10:59:38 PST
Can we identify the changeset that broke this?
Comment 11 :Ms2ger 2011-01-15 11:08:04 PST
That would be http://hg.mozilla.org/mozilla-central/rev/16c93a834c66
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-01-15 11:08:21 PST
(In reply to comment #9)
> (In reply to comment #8)
> > over to js folks then. This is a commonly used API (though I'm not 100% sure
> > how common the () syntax is), so recommending [hardblocker]
> 
> This broke a while ago and it was just reported, so it is not a hardblocker in
> my view -- too much of a corner case of IE bogo-feature emulation. We can fix
> it in a .x release if the patch is safe. But we should see what exactly
> regressed this, in case it broke other things.
> 
> /be

I wouldn't take the fact that it was just reported to mean that it's not a significant regression.  I wouldn't be surprised to find that the set of people using web pages that depend on an archaic IE feature like this and the set of people using our betas do not overlap much.
Comment 13 Andreas Gal :gal 2011-01-15 11:12:53 PST
I will debug through it. The testcase looks simple enough.
Comment 14 :Ms2ger 2011-01-15 11:34:24 PST
<http://software.hixie.ch/utilities/js/live-dom-viewer/saved/779> suggests that, in Fx3.6, we only supported this on document.all and not on the other objects where HTML5 defines it.
Comment 15 Brendan Eich [:brendan] 2011-01-15 12:17:44 PST
(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > over to js folks then. This is a commonly used API (though I'm not 100% sure
> > > how common the () syntax is), so recommending [hardblocker]
> > 
> > This broke a while ago and it was just reported, so it is not a hardblocker in
> > my view -- too much of a corner case of IE bogo-feature emulation. We can fix
> > it in a .x release if the patch is safe. But we should see what exactly
> > regressed this, in case it broke other things.
> > 
> > /be
> 
> I wouldn't take the fact that it was just reported to mean that it's not a
> significant regression.

What's the point of arguing about undefined terms? I said I didn't think this is a hardblocker. If it were the last blocker stopping release, I would not hold for it. That means softblocker?

> I wouldn't be surprised to find that the set of people
> using web pages that depend on an archaic IE feature like this and the set of
> people using our betas do not overlap much.

Could be, and we should fix this if we can. But you did not argue with my case for softblocker directly, instead you introduced "significant regression", an undefined term for which we have zero evidence at hand.

Were that condition to continue and this were the last blocker preventing release, then we would do the release -- and if more evidence of significance came in, we'd do a .x.

/be
Comment 16 Brendan Eich [:brendan] 2011-01-15 12:20:03 PST
(In reply to comment #15)
> hold for it. That means softblocker?

I meant ! not ? there.

(In reply to comment #14)
> <http://software.hixie.ch/utilities/js/live-dom-viewer/saved/779> suggests
> that, in Fx3.6, we only supported this on document.all and not on the other
> objects where HTML5 defines it.

HTML5 should not be mandating other callable collections. I thought we had a deal with Maciej on public-script-coord about this. Cameron, Jonas?

/be
Comment 17 Cameron McCormack (:heycam) 2011-01-15 12:49:31 PST
(In reply to comment #16)
> HTML5 should not be mandating other callable collections. I thought we had a
> deal with Maciej on public-script-coord about this. Cameron, Jonas?

I don't remember a deal with Maciej.  We had agreement from people in the room when we were talking about it in November.  Nobody replied to to my summary email http://lists.w3.org/Archives/Public/public-script-coord/2010OctDec/0094.html saying that callable (among others) would be marked as not suitable for new APIs.

Agreement amongst ourselves and what Ian puts into HTML5 are two different things. :-)  I suspect that Ian considers the callability of those collection objects as falling into the "for compatibility for old stuff" class, rather than new APIs.  (Although I tried to get callability of the new HTMLPropertiesCollection removed: http://www.w3.org/Bugs/Public/show_bug.cgi?id=11032)
Comment 18 Jonas Sicking (:sicking) 2011-01-15 16:46:48 PST
This bug is only about document.all, where we did support callable in 3.6 and so clearly is a regression.

I'll avoid commenting on the callability of other collections as that is off-topic here.
Comment 19 Andreas Gal :gal 2011-01-16 15:19:39 PST
Simplified test case:

javascript:try { document.all("foo") } catch(e) { alert(e); }
Comment 20 Andreas Gal :gal 2011-01-16 16:38:15 PST
Created attachment 504325 [details] [diff] [review]
patch

The embedding asks whether typeof(callee) == "function" to determine whether its the document.all.item case or document.all. We used to reply "object" for document.all, even though its callable. However, ECMA 262, 11.4.3 says that any native object that implements [[Call]] should be of type "function". This broke when we fixed that bug. The JS engine already lies for RegExp, which is callable, but we catch that case in js_TypeOf and report "object" instead. For this case I think we should just change the embedding. The attached patch looks at the class of callee, which seems saner anyway.
Comment 21 Brendan Eich [:brendan] 2011-01-16 17:38:13 PST
Comment on attachment 504325 [details] [diff] [review]
patch

(In reply to comment #20)
> Created attachment 504325 [details] [diff] [review]
> patch
> 
> The embedding asks whether typeof(callee) == "function" to determine whether
> its the document.all.item case or document.all. We used to reply "object" for
> document.all, even though its callable. However, ECMA 262, 11.4.3 says that any
> native object that implements [[Call]] should be of type "function".

But document.all and other DOM objects are not "native objects" by ECMA-262's arcane vocabulary -- they are "host objects".

Even with that jargon correction, ES5 now says typeof ought to return "function" for a host object that implements [[Call]], but ES1-3, which ruled for 13 years, say "Object (host) - Implementation-dependent". The notorious example is alert in IE's DOM until IE9: typeof alert == "object".

Anyway, yay for conforming to ES5, but what else did that change break?

> This broke
> when we fixed that bug. The JS engine already lies for RegExp, which is
> callable, but we catch that case in js_TypeOf and report "object" instead.

Callable regexps are a bug -- we are in long-standing (preceding ES3, which introduced standardized RegExps based on the Netscape implementation) violation of the spec. See bug 582717 (see also the WebKit bug https://bugs.webkit.org/show_bug.cgi?id=28285).

> For
> this case I think we should just change the embedding. The attached patch looks
> at the class of callee, which seems saner anyway.

Hmm, that does not match the comments or the cases described in the comments:

>diff --git a/dom/base/nsDOMClassInfo.cpp b/dom/base/nsDOMClassInfo.cpp
>--- a/dom/base/nsDOMClassInfo.cpp
>+++ b/dom/base/nsDOMClassInfo.cpp
>@@ -8925,29 +8925,26 @@ nsHTMLDocumentSH::CallToGetPropMapper(JS
>   // Convert all types to string.
>   JSString *str = ::JS_ValueToString(cx, JS_ARGV(cx, vp)[0]);
>   if (!str) {
>     return JS_FALSE;
>   }
> 
>   JSObject *self;
> 
>-  if (::JS_TypeOfValue(cx, JS_CALLEE(cx, vp)) == JSTYPE_FUNCTION) {
>-    // If the callee is a function, we're called through
>-    // document.all.item() or something similar. In such a case, self
>-    // is passed as obj.
>-
>+  if (JSVAL_IS_OBJECT(JS_CALLEE(cx, vp)) &&
>+      ::JS_GetClass(cx, JSVAL_TO_OBJECT(JS_CALLEE(cx, vp))) == &sHTMLDocumentAllClass) {
>+    // If we are not called through document.all.item() or something similar,
>+    // self is passed as callee.

This code is more precise about the odd case of document.all(...), but please change the comment to say "If we are called via document.all(id) instead of document.all.item(i) or another method, use the document.all callee object as self."

r=me with that.

/be
Comment 22 Andreas Gal :gal 2011-01-16 23:31:28 PST
http://hg.mozilla.org/tracemonkey/rev/4b1aeca1d4f6
Comment 23 Andreas Gal :gal 2011-01-16 23:31:42 PST
This needs beta coverage.
Comment 24 Chris Leary [:cdleary] (not checking bugmail) 2011-01-18 17:00:27 PST
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/4b1aeca1d4f6
Comment 25 Boris Zbarsky [:bz] 2012-04-21 01:18:31 PDT
Looks like a test for this never landed, and predictably this regressed.  See bug 747617.

Note You need to log in before you can comment on or make changes to this bug.