Last Comment Bug 390411 - Can't override getElementsByClassName by setting it on HTMLElement.prototype
: Can't override getElementsByClassName by setting it on HTMLElement.prototype
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
http://panel.dreamhost.com/
: 399334 409571 789088 (view as bug list)
Depends on: ParisBindings
Blocks: 357450 iWeb
  Show dependency treegraph
 
Reported: 2007-07-31 20:29 PDT by Ryan VanderMeulen [:RyanVM]
Modified: 2015-11-17 11:36 PST (History)
20 users (show)
mtschrep: blocking1.9-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
ajaxform.js (9.87 KB, text/plain)
2007-07-31 20:29 PDT, Ryan VanderMeulen [:RyanVM]
no flags Details
prototype.js (93.79 KB, text/plain)
2007-07-31 21:02 PDT, Ryan VanderMeulen [:RyanVM]
no flags Details
scriptaculous.js (2.60 KB, text/plain)
2007-07-31 21:03 PDT, Ryan VanderMeulen [:RyanVM]
no flags Details

Description Ryan VanderMeulen [:RyanVM] 2007-07-31 20:29:13 PDT
Created attachment 274719 [details]
ajaxform.js

When trying to submit a new support request in the Dreamhost control panel, I receive the following error in the console which prevents me from continuing past the first page.

Security Error: Content at https://secure.newdream.net/newpanel/js/ajaxform.js may not load data from https://panel.dreamhost.com/index.cgi?tree=support.msg&.

Followed by:
Error: this.divid.getElementsByClassName("buttonbox").each is not a function
Source File: https://secure.newdream.net/newpanel/js/ajaxform.js
Line: 33

On the off chance it's relevant, I'm attaching ajaxform.js as well.
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-07-31 20:34:01 PDT
We need a full testcase here. The js file doesn't help much without knowing what page it operates on.

And why is this filed on the XBL component?
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2007-07-31 20:34:36 PDT
What makes you think it's an XBL issue?
Comment 3 Ryan VanderMeulen [:RyanVM] 2007-07-31 20:51:52 PDT
This regressed between the 2007-02-01 and 2007-02-02 nightlies. Regression range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-02-01+04%3A00%3A00&maxdate=2007-02-02+10%3A00%3A00&cvsroot=%2Fcvsroot

I have no idea what component this should be filed under. I guess if we can decide what caused it, it can be changed then.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2007-07-31 20:59:04 PDT
Here's a guess: whatever JS library Dreamhost uses adds getElementsByClassName to Element.prototype, *but only if it's not defined.*  The object returned by the getElementsByClassName provided by Dreamhost is a JS array.  If Dreamhost uses a JS library like, say, Prototype, that adds properties to Array.prototype (in this case a property "each"), then Dreamhost's getElementsByClassName would have a working each method because its return values have Array.prototype as their prototype.  Firefox's, however, would be NodeLists, and whatever JS library Dreamhost uses doesn't add "each" to NodeList.

This shouldn't be too hard to check -- just open Dreamhost's JS files, search for getElementsByClassName, see the conditions under which it's set and the type of its return value, and do the same for Array and each.
Comment 5 Ryan VanderMeulen [:RyanVM] 2007-07-31 21:02:42 PDT
Created attachment 274721 [details]
prototype.js

So I think the security warning is irrelevant to this bug.
Comment 6 Ryan VanderMeulen [:RyanVM] 2007-07-31 21:03:09 PDT
Created attachment 274722 [details]
scriptaculous.js
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2007-07-31 22:00:35 PDT
So prototype.js redefines getElementsByClassName on HTMLElement.prototype, and then it calls that function and expects it to work.  However, from what Ryan says this overriding doesn't actually work, because if runs:

javascript:alert(document.body.getElementsByClassName);

...he gets the [native code] version.  Judging by the following URL, it looks like the override on HTMLElement.prototype isn't working:

data:text/html,<script>window.onload%20=%20function()%20{%20alert(HTMLElement.prototype.getElementsByClassName);%20HTMLElement.prototype.getElementsByClassName%20=%20function%20(name)%20{%20return%2012;%20};%20alert(HTMLElement.prototype.getElementsByClassName);%20alert(document.body.getElementsByClassName("foo"));%20};%20</script>

According to bz, this is because all the methods live on the "most-derived" interface, in this case most likely HTMLDivElement at a guess, so the HTMLElement version doesn't shine through (and never will, if we define getElementsByClassName natively).

I tend to think this is a site bug (and that Prototype shouldn't do what it's doing), but others might think otherwise.
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2007-07-31 22:17:46 PDT
Moving to a slightly more reasonable component for now...
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-08-01 08:52:54 PDT
It does suck though that you can't override stuff by setting it on HTMLElement.property. That's definitely how I would have expected it to work. Is there any chance we can get that fixed, or is that a moz2.0 project?
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2007-08-01 09:20:25 PDT
There've been some e-mail discussions about it, including a thread on webapi, as I recall.  And yes, we should get it fixed.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-08-04 20:38:07 PDT
Is there an explanation for the "regression" mentioned in comment 3? The "can't override methods using HTMLElement.prototype" issue that this bug seems to have morphed into isn't a regression. Was that comment only about the security warning?
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-08-04 23:17:14 PDT
The "regession" was that we implemented a function with the same name as the site was already using, thereby hiding the site version with our own.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2007-08-06 16:53:35 PDT
Not a blocker. And besides, fixing this is really hard (TM), we'd need to redesign how XPConnect exposes prototype objects etc.
Comment 14 Dão Gottwald [:dao] 2007-10-11 02:11:07 PDT
*** Bug 399334 has been marked as a duplicate of this bug. ***
Comment 15 Brendan Eich [:brendan] 2007-10-17 19:12:53 PDT
Is this really that hard to fix? Just asking. I think we want to change our DOM to do what Webkit does, FWIW. See

http://lists.w3.org/Archives/Public/public-webapi/2007Jun/0025.html

/be
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2007-10-17 20:04:06 PDT
> Is this really that hard to fix?

Right now XPConnect puts all the methods it exposes on the immediate prototype of the XPCWrappedNative.  That is, that's where it installs its various hooks, etc.

Changing that in an alpha might be doable.  Changing it now is Hard.

If we do want to fix it, we should remove the XPConnect IDL definition (or make it noscript) and implement this in classinfo somehow (and try to detect the function up the proto chain as needed).
Comment 17 Shawn Wilsher :sdwilsh 2007-12-22 13:14:56 PST
*** Bug 409571 has been marked as a duplicate of this bug. ***
Comment 18 Mike Schroepfer 2008-01-23 17:00:56 PST
This + getElementByClassName looks to have broken apple.com (bug 412150).  Is there some other way around this?
Comment 19 Robert Sayre 2008-01-23 17:02:53 PST
Yeah, could we special-case this method somehow?
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2008-01-23 17:20:52 PST
Bug 412150 is a different issue.  It's using a framework (prototype.js) which does:

  if (!document.getElementsByClassName) 
    document.getElementsByClassName = function(instanceMethods){

etc.  So fixing this bug would have no effect on apple.com.

It's too bad the edge case behavior of prototype's getElementsByClassName is so different.
Comment 21 Mike Schroepfer 2008-01-23 17:39:41 PST
Removing my own nom request based on comment 20
Comment 22 James Justin Harrell 2008-03-27 06:59:53 PDT
If the ability to override (or even just extend) DOM types, by modifying/adding properties to their prototype chains, is important to interoperability with other browsers, some specification needs to define how it should work. I believe IE doesn't (or didn't) allow it all, and they aren't violating any spec by not allowing it (besides maybe the ECMAScript specs).

The most likely candidate for this is probably HTML5, although it doesn't have a nice section on ECMAScript bindings like the W3C specs do, e.g.
http://www.w3.org/TR/DOM-Level-3-Core/ecma-script-binding.html

If this isn't considered important to interoperability, this bug should be marked invalid. If this is considered import to interoperability, we should work to get this specified in detail and then conform to those specifications.

Also, I'm wondering how this will be affected by JavaScript 2 (ECMAScript 4). When Mozilla gets that rolled out, will DOM types be defined using classes and/or interfaces? Would that affect the prototype chain in some way?
Comment 23 Patrick 2008-06-05 00:25:18 PDT
would this fix bug 405903? (ref bug 405903 comment 29)
Comment 24 Cyrus Omar 2008-07-24 01:33:23 PDT
Interestingly, the following code works:

document.createElement('div') instanceof HTMLElement
// returns true

Yet nowhere in the prototype chain of that element is there HTMLElement.prototype:

var found = false
var div = document.createElement('div')
while (div = div.__proto__) {
  if (div == HTMLElement.prototype) found = true
}
// found === false at the end

Maybe this is a new bug, but instanceof should NOT be returning true if the corresponding function's prototype is not in the prototype chain of the object. 

As far as overriding properties goes, you can solve this by going through all the derivative classes (HTMLDivElement, etc.) and setting the override on each of them instead of at the HTMLElement level. If you want to add _new_ properties to all HTMLElements, it also seems to work to go through each derivative class and set __proto__ to HTMLElement.prototype first.
Comment 25 Peter Van der Beken [:peterv] 2008-07-24 09:26:58 PDT
(In reply to comment #24)
> var found = false
> var div = document.createElement('div')
> while (div = div.__proto__) {
>   if (div == HTMLElement.prototype) found = true
> }
> // found === false at the end

This works for me (found === true for me).
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2008-07-24 10:05:58 PDT
> instanceof should NOT be returning true if the corresponding function's
> prototype is not in the prototype chain

That's not workable given that the DOM involves multiple inheritance.  But in any case, HTMLElement.prototype is in fact in the prototype chain for a <div>.
Comment 27 Cyrus Omar 2008-07-24 11:09:59 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > var found = false
> > var div = document.createElement('div')
> > while (div = div.__proto__) {
> >   if (div == HTMLElement.prototype) found = true
> > }
> > // found === false at the end
> 
> This works for me (found === true for me).
> 

Hmm it also works for me, I ran that in a console after messing around with some stuff, I probably used an '=' somewhere instead of '=='. My fault, please disregard my comment.
Comment 28 Cyrus Omar 2008-07-24 11:15:03 PDT
(In reply to comment #26)
> > instanceof should NOT be returning true if the corresponding function's
> > prototype is not in the prototype chain
> 
> That's not workable given that the DOM involves multiple inheritance.  But in
> any case, HTMLElement.prototype is in fact in the prototype chain for a <div>.
> 

We shouldn't be changing the semantics of ECMAScript to suit bindings to an external library, even if its an important one like DOM. ECMAScript3 (and 4 incidentally) say instanceof should follow the prototype chain. If there is an interface or something that is implemented by the object but not part of the prototype chain, instanceof should return false. Are there any examples where it currently returns true (my HTMLElement example doesn't count anymore :))
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2008-07-24 12:06:48 PDT
> ECMAScript3 (and 4 incidentally) say instanceof should follow the prototype
> chain.

Oh, really?  Ignoring for the moment that all this is completely off-topic for this bug, ES3 has the following to say, in section 11.8.6:

  The production Relational Expression: Relational Expression instanceof Shift
  Expression is evaluated as follows: 
  1. Evaluate Relational Expression. 
  2. Call GetValue(Result(1)). 
  3. Evaluate Shift Expression. 
  4. Call GetValue(Result(3)). 
  5. If Result (4) is not an object, throw a TypeError exception. 
  6. If Result (4) does not have a [[HasInstance]] method, throw a TypeError
     exception. 
  7. Call the [[HasInstance]] method of Result(4) with parameter Result(2). 
  8. Return Result(7) . 

See section 8.6.2 for the description of [[HasInstance]] and note the "host objects may implement these internal methods with any implementation-dependent behavior" bit.  I assure you that HTMLElement.prototype is a host object, not a native ECMAScript object.

But in any case, there is no mention of the prototype chain anywhere here, and in fact per ES3 spec something like |{} instanceof Object.prototype| should throw TypeError (and does, in Gecko).  Something you could have easily verified before making a clearly false statement like "instanceof should follow the prototype chain".

I see nothing obvious in ES4 that changes any of this.
Comment 30 Cyrus Omar 2008-07-24 12:24:15 PDT
I just Googled "ECMAScript 3 instanceof" and found the following quote in the spec for ES4 relational operators:

"The instanceof operator behaves in the same way as in ECMAScript 3 — a instanceof b follows a’s prototype chain."
http://docs.huihoo.com/web/js/es4/core/expressions.html

I assumed that the page claiming to be an ES4 spec was making a correct statement about the ES3 spec, though looking more closely, this is Netscape's ES4 spec? I'm not familiar enough with the progress of ES4 to independently verify the claims on the page or whether they have changed since that page was written, so I'll defer to you on that. However, I didn't just make something up, thanks.

As far as |{ } instanceof Object.prototype| goes, Object.prototype is an object, not a function, so it should certainly throw an error. |{ } instanceof Object| however, should return true (and does.)

In any case, thanks for the clarification and sorry for the confusion on the bug report.
Comment 31 Brendan Eich [:brendan] 2009-10-05 21:46:59 PDT
(In reply to comment #16)
> > Is this really that hard to fix?
> 
> Right now XPConnect puts all the methods it exposes on the immediate prototype
> of the XPCWrappedNative.  That is, that's where it installs its various hooks,
> etc.
> 
> Changing that in an alpha might be doable.  Changing it now is Hard.

How about now (mozilla-central for 3.7)?

> If we do want to fix it, we should remove the XPConnect IDL definition (or make
> it noscript) and implement this in classinfo somehow (and try to detect the
> function up the proto chain as needed).

Isn't a sufficient fix to find the class's prototype, or in multiple-inheritance cases the next concrete class down, and define the method there? Maciej's recap of what WebKit does (or did) from

http://lists.w3.org/Archives/Public/public-webapi/2007Jun/0025.html

"Generally, we make the same prototype chain you describe, but we put  
methods on the JS [[Class]] that defines them. In cases of multiple  
interfaces or multiple inheritance, we fold the set of methods into  
the next concrete class to inherit from the interfaces."

Sounds doable, not saying it's easy.

Why would the IDL definition go away or become noscript?

/be
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2009-10-06 07:33:45 PDT
> How about now (mozilla-central for 3.7)?

If we have someone (or two) to do the work and a plan for how to handle the multiple-inheritance situation, sure.  We're in an alpha cycle, after all.

> Why would the IDL definition go away or become noscript?

That suggestion was scoped in a "if we want to fix this without fundamental changes to XPConnect" block.

I have no idea what Maciej means by "fold".  What's needed to fix this, in general, is lookup along an ancestor tree (due to multiple inheritance), not ancestor chain.  That certainly seems to be the way Webkit does it, one way or another.
Comment 33 Ryan VanderMeulen [:RyanVM] 2012-02-18 08:50:16 PST
Is this bug still relevant?
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2012-02-18 17:41:52 PST
Yes, though bug 580070 will fix it.
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2012-09-06 11:19:46 PDT
*** Bug 789088 has been marked as a duplicate of this bug. ***
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2015-11-17 11:36:15 PST
Fixed by putting all Nodes on Web IDL bindings.  Not going to worry about looking up that bug number...

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