1081037 - ctor function returned from registerElement does not have a prototype

Status: RESOLVED FIXED

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

Description

[Gabor Krizsanits (INACTIVE)]

var Foo = document.registerElement('x-foo', {
  prototype: {bar: 5}
});

Foo.prototype.bar = 6;

Exception: Foo.prototype is undefined

instancof might fail too because of this...

Comment 1

[Boris Zbarsky [:bzbarsky]]

The spec here needs to be clarified.

Specifically, all it says is:

  Let CONSTRUCTOR be the interface object whose interface prototype object is PROTOTYPE

What this would normally mean in Web IDL is the following:

  CONSTRUCTOR.prototype == PROTOTYPE
  PROTOTYPE.constructor == CONSTRUCTOR

but these are static constraints in Web IDL, since the set of interfaces is fixed.

For the registerElement case, what happens if the same prototype is registered multiple times?  What does its .constructor end up being?  Futhermore, what should happen if the .constructor is non-configurable, or nonwritable, or both, before the registerElement call happens?  Is the property set via Put() or via  DefinePropertyOrThrow() or something else?

I mean, in terms of implementation we can just JS_LinkConstructorAndPrototype and move on for now, but that may not give us the same behavior that Chrome has, and per spec the behavior is just not defined.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=27016 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=27017

Comment 3

[Cheba]

Not sure if there was any changes to this recently but since Friday (Oct 31) GitHub's JS is broken because of this when webcomponents are enabled in the Nightly.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Cheba from comment #3)
> Not sure if there was any changes to this recently but since Friday (Oct 31)
> GitHub's JS is broken because of this when webcomponents are enabled in the
> Nightly.

Are you sure it's because of this issue? I don't think we changed anything in this area yet (waiting for the spec to be sorted out), maybe something changed on GitHub?

Comment 5

[Cheba]

Bad wording.

I just wanted to say that I had webcomponents preffed on for quite some time and it's only broken since last Friday.

I don't know what actually changed (FF or GH's JS) just saying that now it doesn't work and this is the bug I'm hitting.

Comment 7

[Wilson Page [:wilsonpage]]

This Twitter thread [1] adds a bit more context. This seems like something that could be fixed pretty easily. I can't think of any reason why we wouldn't want to expose the prototype. This is inline with Chrome's implementation.

[1] https://twitter.com/wilsonpage/status/529243471362916352

Comment 8

[Joshua Peek]

On the GitHub side, I recently just deployed some new code that made use of the exported ctor prototype. We where using custom elements before that, but a few days ago was the first usage of this specific api.

I suppose I can patch it for now by doing something like:

   window.FooElement = document.registerElement('x-foo', {prototype: FooElementProto)
   window.FooElement.prototype = FooElementProto

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> I mean, in terms of implementation we can just
> JS_LinkConstructorAndPrototype and move on for now, but that may not give us
> the same behavior that Chrome has, and per spec the behavior is just not
> defined.

Should we just do this for now until the spec is fixed? (I would not hold my breath...)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, agreed (esp. on not holding your breath).  Please add some testcases for the following edge cases, though:

1)  The "prototype is already an interface prototype object" case from the spec, both for the case of a built-in interface prototype and for the case of one registered via this API.  File a bug on Chrome if it fails this test.

2)  The case when PROTOTYPE has a non-configurable writable property named "constructor", which per spec should throw NotSupportedError.  Again, file a bug on Chrome if it fails this test.

3)  The case when PROTOTYPE has a non-configurable readonly property named "constructor" (should still throw).  Again, bug as needed.

4)  The case when PROTOTYPE has a configurable readonly property named "constructor"; this should presumably reconfigure the property to be writable and change its value to the new constructor.  Add a comment in the test about how this behavior is not defined in the spec.

5)  The case when PROTOTYPE is a proxy that reports no "constructor" property but throws when attempting to define such a property on the object.  This should presumably cause the whole thing to throw; worth filing a bug on Chrome if it doesn't.  Add a comment in the test about how this behavior is not defined in the spec.

6)  The case when PROTOTYPE is a proxy that has side-effects in both property lookup and property definition; just make this test claim whatever behavior we have no is correct but add a comment about how the behavior is not defined in the the spec.

Plus of course a test that the "prototype" of the ctor is set up correctly (non-configurable, non-enumerable, non-writable, and again a bug on Chrome if that's not the case there).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, and regarding comment 7: _something_ can be fixed pretty easily here.  Fixing the real issue, which is that the spec is totally borked, can't be done easily because the spec editor refuses to admit there is even a problem.

Comment 12

[Cheba]

(In reply to Boris Zbarsky [:bz] from comment #10)
> File a bug on Chrome if it fails this test.

Just curious. Shouldn't a bug be filed on IE, too?

Comment 13

[Boris Zbarsky [:bzbarsky]]

IE doesn't have support for custom elements as far as I know.  We're the only ones dumb enough to be trying to implement yet another Google-authored not-really-spec.

Comment 14

[Gabor Krizsanits (INACTIVE)]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #10)> 
> 1)  The "prototype is already an interface prototype object" case from the
> spec, both for the case of a built-in interface prototype and for the case
> of one registered via this API.  File a bug on Chrome if it fails this test.

We don't check for the "registered via this API" case in RegisterElement yet.
And I don't know how should we do it. In the other case we just check for a
flag in the class of the proto, but in the custom element case we don't have
any classp to flag... We could try to look up the proto in the map, but it
can be a custom element from another document... Can we somehow flag all
the proto objects we use for element registration? Or should I just check
if the ctor of the proto has a native that points to nsDocument::CustomElementConstructor?
(this version seems a bit hacky and not quite bulletproof)

Comment 15

[Boris Zbarsky [:bzbarsky]]

> We don't check for the "registered via this API" case in RegisterElement yet.

Sure, so the testcase would need a todo().

> And I don't know how should we do it.

I'm not asking you to implement it here, just to have a test and followup bug.  Sorry that wasn't clear.  We can figure out how to implement it in that bug.

Comment 16

[Gabor Krizsanits (INACTIVE)]

Attachment: LinkConstructorAndPrototype in RegisterElement. v1

We already had some of the mentioned tests, I've added the ones I could not find elsewhere. Found some bugs in google chrome as well, will report those bugs as well soon. (I just need a different env. for those since annoyingly canary does not support linux...)

Comment 17

[Gabor Krizsanits (INACTIVE)]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #15)
> just to have a test and followup bug

Alright, I'll filed bug 1103933, and added a todo() in the test for now.

Comment 18

[William Chen [:wchen]]

Comment on attachment 8527621 [details] [diff] [review]
LinkConstructorAndPrototype in RegisterElement. v1

Review of attachment 8527621 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +6216,5 @@
>    }
>  
> +  JS::Rooted<JSObject*> constructorObj(aCx, JS_GetFunctionObject(constructor));
> +  if (!JS_LinkConstructorAndPrototype(aCx, constructorObj, protoObject)) {
> +    rv.Throw(NS_ERROR_OUT_OF_MEMORY);

This doesn't look like the right error to throw here. NS_ERROR_DOM_NOT_SUPPORTED_ERR?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> This doesn't look like the right error to throw here.

In practice, it doesn't matter.  If JS_LinkConstructorAndPrototype returned false, it already threw an exception on aCx.

Comment 20

[Gabor Krizsanits (INACTIVE)]

It took me a while to re-enable the right web-platform tests...

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e462d4c277e2

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d3fe12c63ed

Comment 21

[Carsten Book [:Tomcat]]

sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4552087&repo=mozilla-inbound

Comment 22

[Gabor Krizsanits (INACTIVE)]

It seems to me that Gaia already did a workaround for this bug. Things like: http://mxr.mozilla.org/gaia/source/apps/camera/bower_components/gaia-component/gaia-component.js#45

And now that line is throwing since the proto is readonly.

Wilson, could you help me out how to land this? This should be fixed in Gaia but I have no idea how to land a patch in Gaia land. Also, I don't know what is the process to sync landing this patch and the one that removes those hacks from Gaia.

Comment 23

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 24

[Guillaume Marty [:gmarty]]

Comment on attachment 8535002 [details] [review]
pull-request (master)

Lookg good to me!

Comment 25

[Wilson Page [:wilsonpage]]

Comment on attachment 8535002 [details] [review]
pull-request (master)

LANDED https://github.com/mozilla-b2g/gaia/commit/2589cd53bb84452eca4865cdd17d634fd344c2f7

Comment 26

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f662a378f18d

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f662a378f18d