Closed
Bug 1081037
Opened 10 years ago
Closed 10 years ago
ctor function returned from registerElement does not have a prototype
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: gkrizsanits, Assigned: gkrizsanits)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(2 files)
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•10 years ago
|
Blocks: webcomponents
Comment 1•10 years ago
|
||
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•10 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•10 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?
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•10 years ago
|
||
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•10 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•10 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...)
Comment 10•10 years ago
|
||
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•10 years ago
|
||
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•10 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?
Comment 13•10 years ago
|
||
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•10 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•10 years ago
|
Assignee: nobody → gkrizsanits
Comment 15•10 years ago
|
||
> 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•10 years ago
|
||
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 | ||
Comment 17•10 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 18•10 years ago
|
||
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+
Comment 19•10 years ago
|
||
> 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•10 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
Comment 21•10 years ago
|
||
sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4552087&repo=mozilla-inbound
Assignee | ||
Comment 22•10 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)
Comment 23•10 years ago
|
||
Flags: needinfo?(wilsonpage)
Attachment #8535002 -
Flags: review?(gmarty)
Attachment #8535002 -
Flags: feedback?(gkrizsanits)
Assignee | ||
Updated•10 years ago
|
Attachment #8535002 -
Flags: feedback?(gkrizsanits) → feedback+
Comment 24•10 years ago
|
||
Comment on attachment 8535002 [details] [review]
pull-request (master)
Lookg good to me!
Attachment #8535002 -
Flags: review?(gmarty) → review+
Comment 25•10 years ago
|
||
Comment on attachment 8535002 [details] [review]
pull-request (master)
LANDED https://github.com/mozilla-b2g/gaia/commit/2589cd53bb84452eca4865cdd17d634fd344c2f7
Assignee | ||
Comment 26•10 years ago
|
||
Comment 27•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•