929554 - Disallow object-valued and any-valued attributes for JS-implemented WebIDL

Status: RESOLVED WONTFIX

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

Description

[Bobby Holley (:bholley)]

smaug points out that the cloning we want to do in bug 923904 violates the semantics of attributes, whereby the value of the attribute should not change on each access (i.e. we should have |foo.attr === foo.attr|).

We've agreed that we should simply disallow implemented such interfaces (which include CustomEvent) in JS. Andrew is going to take the fix here.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Yeah, at least for now we should not support implementing such interfaces in JS, and looks like
no JSImplemented interface has such attributes, luckily. 
We can remove this restriction if we figure out good ways to implement it, or
add some annotation to make it clear that an attribute never returns the same object
(which is kind odd).

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Looks like I missed some cases. We do have some such attributes in JSImplemented interfaces :(

Comment 3

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #2)
> Looks like I missed some cases. We do have some such attributes in
> JSImplemented interfaces :(

Which ones?

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

★ ~/moz/mozilla-central/dom/webidl $ grep object *.webidl | grep attribute
CanvasRenderingContext2D.webidl:  attribute object mozCurrentTransform; // [ m11, m12, m21, m22, dx, dy ], i.e. row major
CanvasRenderingContext2D.webidl:  attribute object mozCurrentTransformInverse;
Contacts.webidl:  attribute object?    type; // DOMString[]
Contacts.webidl:  attribute object?    type; // DOMString[]
Contacts.webidl:           attribute object?      photo;
Contacts.webidl:           attribute object?      adr;
Contacts.webidl:           attribute object?      email;
Contacts.webidl:           attribute object?      url;
Contacts.webidl:           attribute object?      impp;
Contacts.webidl:           attribute object?      tel;
Contacts.webidl:           attribute object?      name;
Contacts.webidl:           attribute object?      honorificPrefix;
Contacts.webidl:           attribute object?      givenName;
Contacts.webidl:           attribute object?      additionalName;
Contacts.webidl:           attribute object?      familyName;
Contacts.webidl:           attribute object?      honorificSuffix;
Contacts.webidl:           attribute object?      nickname;
Contacts.webidl:           attribute object?      category;
Contacts.webidl:           attribute object?      org;
Contacts.webidl:           attribute object?      jobTitle;
Contacts.webidl:           attribute object?      note;
Contacts.webidl:           attribute object?      key;
Document.webidl:  // special event handler IDL attributes that only apply to Document objects
HTMLAppletElement.webidl:           attribute DOMString _object;
HTMLDocument.webidl:  readonly attribute object all;
IDBDatabase.webidl:    readonly    attribute DOMStringList      objectStoreNames;
IDBIndex.webidl:    readonly    attribute IDBObjectStore objectStore;
IDBTransaction.webidl:    readonly    attribute DOMStringList objectStoreNames;
RTCPeerConnection.webidl:  readonly attribute object localStreams;
RTCPeerConnection.webidl:  readonly attribute object remoteStreams;
★ ~/moz/mozilla-central/dom/webidl $

Comment 5

[Bobby Holley (:bholley)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> ★ ~/moz/mozilla-central/dom/webidl $ grep object *.webidl | grep attribute

We need to filter by the ones that are JS-implemented.

Comment 6

[Andrew McCreight [:mccr8]]

I threw together a patch, and it turns out this is used in a few places.

object attributes are used in ContactAddress, ContactField, mozContact and mozRTCPeerConnection.  For mozRTCPeerConnection, it is those obsoleted fields that are going to be removed in bug 929530.  Reuben says that he needed to use object for contacts because of the lack of arrays.

any is used in DOMMMIError.  I'm not sure what that is exactly, but it is a very small interface.  Also in the interface TestJSImplInterface, for the attribute jsonifierShouldSkipThis, which presumably could just be commented out.

It would be nice to add a test for this, but I'm not sure how that would work.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: hacky checker

This only checks getters (checking setters doesn't add any more as you'd expect), and doesn't throw, but just prints out a message, so you can see everything that fails.

It might be nicer to check for this earlier, in the parser or some such, and then just assert here that the types are okay, but maybe it doesn't matter.

Comment 8

[Bobby Holley (:bholley)]

We decided to not do this, and did bug 1036214 instead.