Bug 20225 - don't allow overridden operations and attribute getters/setters to be invoked on descendant objects
don't allow overridden operations and attribute getters/setters to be invoked...
Status: NEW
Product: WebAppsWG
Classification: Unclassified
Component: WebIDL
unspecified
PC Windows 3.1
: P2 normal
: ---
Assigned To: Cameron McCormack
public-webapps-bugzilla
[v1]
:
Depends on:
Blocks: 15200 17201
  Show dependency treegraph
 
Reported: 2012-12-04 00:38 UTC by Cameron McCormack
Modified: 2013-06-17 01:24 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack 2012-12-04 00:38:40 UTC
For cases like HTMLPropertiesCollection.namedItem, which overrides HTMLCollection.namedItem, we don't want to allow HTMLCollection.prototype.namedItem to be called on an HTMLPropertiesCollection object.

 <Hixie> if a method/setter/getter from a prototype of interface A is applied to an object that is of an interface B that is a subclass of A, and B defines its own method/getter/setter with the same name, then throw.
 <Hixie> by setters/getters i meant attributes
Comment 1 Boris Zbarsky 2012-12-04 01:16:34 UTC
This "B defines its own method/getter/setter with the same name" test is not a reasonable one, in my opinion.  It's not really implementable in a particularly performant way when the methods have the same signature, because runtime introspection gets involved....
Comment 2 Cameron McCormack 2012-12-04 01:21:08 UTC
Oh, I don't think it's the intention to look at properties at run time -- just what IDL operations/attributes are declared on interfaces.  So HTMLCollection.prototype.namedItem would always first just check if `this` is an HTMLPropertiesCollection, and throw if so.  Whether there is a property called namedItem somewhere else on the prototype chain at run time shouldn't matter.
Comment 3 Cameron McCormack 2012-12-04 01:22:10 UTC
(Or I might have misinterpreted your point about runtime introspection, since checking if the object is an HTMLPropertiesCollection could count as introspection.)
Comment 4 Boris Zbarsky 2012-12-04 01:39:01 UTC
> So HTMLCollection.prototype.namedItem would always first just check if `this`
> is an HTMLPropertiesCollection, and throw if so.

Yes, that's runtime introspection, no?  Depending on exactly what the sub-set of interfaces that override the property is this could get pretty expensive...

I suppose we could try it in practice and see how much it hurts things, but the check above would about double the cost of calling namedItem on HTMLCollections.
Comment 5 Ian 'Hixie' Hickson 2012-12-05 21:00:47 UTC
I meant this to be entirely a compile-time thing. The implementation of HTMLCollection.namedItem() is going to have to know how to handle being applied to HTMLCollection objects, HTMLPropertiesCollection objects, HTMLElement objects, Date objects, you name it, already. All I'm saying is that when it's applied to HTMLPropertiesCollection objects, instead of trying to do something and potentially crashing because the rest of the code expects HTMLPropertiesCollection objects to have their own namedItem and therefore might not be ready to handle this one, it should just throw.

The alternative is to define how it works on HTMLPropertiesCollection objects, but I don't think that scales.
Comment 6 Boris Zbarsky 2012-12-06 05:47:19 UTC
> The implementation of HTMLCollection.namedItem() is going to have to know how
> to handle being applied to HTMLCollection objects, HTMLPropertiesCollection
> objects, HTMLElement objects, Date objects, you name it, already.

There are two parts here:

1)  The binding code, which implements WebIDL.  This knows how to ensure that the
    method is being applied to an HTMLCollection object (which includes objects
    implementing any interface that inherits from HTMLCollection), period.  It
    does this via runtime introspection of the "this" object, because it has to.
2)  The actual method implementation, which assumes it has an HTMLCollection to
    work with (again, including subclasses).

> All I'm saying is that when it's applied to HTMLPropertiesCollection objects

And all I'm saying is that this changes the binding code from (pseudocode):

  if (object is not HTMLCollection) {
    throw;
  }
  call implementation;

to (still pseudocode)

  if (object is not HTMLCollection or object is HTMLPropertiesCollection) {
    throw;
  }
  call implementation;

which means double the work, and that work is one of the most expensive parts of this call to start with.  Maybe that's an acceptable cost, but it's not one we should just blindly take.

> because the rest of the code 

Which code?  Are you talking about the binding code, the implementation code, web page code, something else?

> The alternative is to define how it works on HTMLPropertiesCollection objects

Yes.

> but I don't think that scales.

Why not?
Comment 7 Boris Zbarsky 2012-12-06 05:51:52 UTC
One note on terminology: I'm using "runtime" as "while rendering the webpage" whereas "compile-time" is "while creating that thing the user will download and use as a browser".  Just to make sure we're not talking about JIT-compile time here, because in fact some of these checks _can_ be made at JIT-compile time, and in SpiderMonkey's case we do just that for the "this" object in the common case.
Comment 8 Ian 'Hixie' Hickson 2012-12-07 01:46:04 UTC
I don't distinguish binding code from implementation code; it's all one binary, and some implementations don't have an explicit "binding" anyway. The check about whether it's an HTMLCollection or HTMLPropertiesCollection has to be somewhere. Whether it's in the implementation of the method or the binding, it's the same cost; in theory, one pointer comparison.

By "I don't think that scales" I mean that it would mean that every time an interface wants to specialise a base class' behaviour, the spec would have to define what the superclass' method would do when applied to a subclass.
Comment 9 Boris Zbarsky 2012-12-07 02:15:03 UTC
> I don't distinguish binding code from implementation code

I do, because the difference is web-detectable because it affects the ordering of argument conversions (which can throw) and the execution of whatever special check you're talking about.

> and some implementations don't have an explicit "binding" anyway.

Can you name one?  The ones I'm familiar with, both firsthand and secondhand do have such a thing.  And WebIDL clearly defines that there's the code that gets run before you call into the actual method implementation, and then there's the method implementation.

> The check about whether it's an HTMLCollection or HTMLPropertiesCollection has
> to be somewhere.

Why?

If you have an object whose proto chain includes HTMLCollection.prototype, why exactly do the methods from that prototype not Just Work on that object?  If it's really different enough that this is the case, I would argue it shouldn't have that prototype on its proto chain.

> Whether it's in the implementation of the method or the binding, it's the same
> cost

This happens to not be true in any of the binding systems I've seen.

It's trivial to do in the implementation of the method, because in the method your can have the HTMLCollection.prototype.namedItem method have a quick way to check what object it's working on and HTMLPropertiesCollection objects 

> in theory, one pointer comparison

I don't see how, but please do feel free to enlighten me.

> I mean that it would mean that every time an interface wants to specialise a
> base class' behaviour

Why exactly would it want to do that?  That's the part I don't get.  If you want a different behavior from the other thing, why are you inheriting from the other thing?

In particular, in this case the HTMLCollection behavior is that namedItem() returns null for unsupported names.  You seem to want to override that with a namedItem which never returns null (except is declared as returning null sometimes in the IDL?), right?  Why, exactly?  And if you _do_ want your behavior to be that different, in what sense are you an HTMLCollection?  Just so you can reuse its dynamic updating behavior definition?

My point is that this sort of random overriding is bad enough API design that we _should_ be making it hard to write specs like this.  In my opinion.

> the spec would have to define what the superclass' method would do when
> applied to a subclass

Yes, and that seems fine to me, personally.
Comment 10 Boris Zbarsky 2012-12-07 03:06:57 UTC
So one more note.  The real issue here is whether we think it will be common for spec authors to have good reasons to override proto methods/properties.

If we think it will, and that this is desirable, then I agree we should have some sort of default behavior for that situation.

But I have yet to see a good use of such overriding...
Comment 11 Ian 'Hixie' Hickson 2012-12-07 03:17:57 UTC
If the methods as defined just work, then I'm fine with that. I thought it had been established that they didn't. If they do, isn't the spec already fine?
Comment 12 Boris Zbarsky 2012-12-07 03:25:15 UTC
The method as defined at http://dom.spec.whatwg.org/#dom-htmlcollection-nameditem makes sense for an HTMLPropertiesCollection as far as it goes, no?

But note that HTMLCollection defines the return value to be "object" precisely so that people who want to define other behavior can.  If we're not planning to make use of that functionality, we should change HTMLCollection to just return Element? from namedItem and let places that want to return something else override the method as you're doing here.

Basically, either we should be using the extensibility hook that DOMCore already provides here or we should stop providing that hook if it has no consumers.  The current situation leads to confusion because people have to start worrying about why the hook is being provided and whether they're reading the spec right.
Comment 13 Boris Zbarsky 2012-12-07 03:28:13 UTC
Note that HTMLFormControlsCollection has a similar issue, by the way.

Of course an open question is what behavior users would expect for these collections if they change the value of HTMLCollection.prototype.namedItem.  Would they expect that to be visible on HTMLFormControlsCollection and HTMLPropertiesCollection?
Comment 14 Ian 'Hixie' Hickson 2012-12-07 21:51:40 UTC
Well to be frank I don't really have an opinion on these things. I am happy to change the spec one way or another if that's the better solution than a WebIDL change.

(BTW, the other issue here is onerror on HTMLBodyElement and HTMLFramesetElement.)
Comment 15 Boris Zbarsky 2012-12-07 21:57:07 UTC
That one would also Just Work: it would add an error handler to the element itself.  It's just that there the spec's general definition of how these things work is so confusing that actually figuring that out is impossible...
Comment 16 Anne 2012-12-28 12:47:52 UTC
I'd be happy to change namedItem() on HTMLCollection back to 'Element?'.

I also agree that subclassing with different behavior for some methods seems bad. It might be better to just duplicate the interface in such a scenario. If it's common we could have something like this

interface HTMLPropertiesCollection = HTMLCollection {
  // except for these members
  ... 
}

but I doubt we'd need this often.
Comment 17 Cameron McCormack 2013-01-07 23:42:45 UTC
It is probably easier to solve this without a Web IDL solution, and just avoid defining interfaces that cause operations/attributes from ancestor interfaces to not work on them.  (I changed my mind about not allowing overridden operations to be invoked on descendant objects -- I think (again) it is just poor interface design.)

(In reply to comment #16)
> I'd be happy to change namedItem() on HTMLCollection back to 'Element?'.

The two reasonable solutions that don't involve changing Web IDL are (1) to make HTMLCollection work use 'object?', and define the interface to be a more general collection, and particular instances define what items go in the collection, and (2) to change namedItem() to 'Element?' and have HTMLPropertiesCollection not inherit from HTMLCollection.

I'd be happy with either.