ctor function returned from registerElement does not have a prototype

RESOLVED FIXED in mozilla37

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: krizsa, Assigned: krizsa)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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...
(Assignee)

Updated

3 years ago
Blocks: 811542
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.
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

3 years ago
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.
(Assignee)

Comment 4

3 years ago
(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

3 years ago
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.
Duplicate of this bug: 1048339
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

3 years ago
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
(Assignee)

Comment 9

3 years ago
(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...)
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).
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

3 years ago
(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?
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.
(Assignee)

Comment 14

3 years ago
(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)
(Assignee)

Updated

3 years ago
Assignee: nobody → gkrizsanits
> 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.
(Assignee)

Comment 16

3 years ago
Created attachment 8527621 [details] [diff] [review]
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...)
Attachment #8527621 - Flags: review?(wchen)
(Assignee)

Updated

3 years ago
Depends on: 1103933
(Assignee)

Comment 17

3 years ago
(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 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?
Attachment #8527621 - Flags: review?(wchen) → review+
> 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.
(Assignee)

Comment 20

3 years ago
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
sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4552087&repo=mozilla-inbound
(Assignee)

Comment 22

3 years ago
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.
Flags: needinfo?(wilsonpage)
Created attachment 8535002 [details] [review]
pull-request (master)
Flags: needinfo?(wilsonpage)
Attachment #8535002 - Flags: review?(gmarty)
Attachment #8535002 - Flags: feedback?(gkrizsanits)
(Assignee)

Updated

3 years ago
Attachment #8535002 - Flags: feedback?(gkrizsanits) → feedback+
Comment on attachment 8535002 [details] [review]
pull-request (master)

Lookg good to me!
Attachment #8535002 - Flags: review?(gmarty) → review+
Comment on attachment 8535002 [details] [review]
pull-request (master)

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

Comment 26

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f662a378f18d
https://hg.mozilla.org/mozilla-central/rev/f662a378f18d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

Updated

3 years ago
Duplicate of this bug: 1119745
You need to log in before you can comment on or make changes to this bug.