Closed Bug 626050 Opened 14 years ago Closed 14 years ago

document.all() doesn't work, even in quirks mode

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: rain1, Assigned: gal)

References

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey, hardblocker)

Attachments

(2 files)

Attached file test case (deleted) —
(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.
I guess this isn't really worth blocking for, but there's no harm asking.
blocking2.0: --- → ?
Is this a regression? If so, it would be great to have regression range.
Yes, it's a regression. It works in Firefox 3.6.13. I guess I should start bisecting...
I thought .all used [] not (), but apparently both need to be supported.
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?
Summary: document.all doesn't work, even in quirks mode → document.all() doesn't work, even in quirks mode
(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.
Version: unspecified → Trunk
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]
Assignee: nobody → general
blocking2.0: ? → final+
Component: DOM → JavaScript Engine
Keywords: regression
QA Contact: general → general
(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
Assignee: general → gal
Blocks: 547046
Can we identify the changeset that broke this?
Keywords: qawanted
(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.
I will debug through it. The testcase looks simple enough.
<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.
(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
(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
(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)
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.
Simplified test case: javascript:try { document.all("foo") } catch(e) { alert(e); }
Attached patch patch (deleted) — Splinter Review
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.
Attachment #504325 - Flags: review?(brendan)
Attachment #504325 - Flags: review?(jst)
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
Attachment #504325 - Flags: review?(brendan) → review+
Keywords: qawanted
Whiteboard: fixed-in-tracemonkey
This needs beta coverage.
blocking2.0: final+ → betaN+
Flags: in-testsuite?
Attachment #504325 - Flags: review?(jst) → review+
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey, hardblocker
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 747617
Looks like a test for this never landed, and predictably this regressed. See bug 747617.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: