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)

defect
Not set
normal

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...
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.
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.
(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.
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
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
(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.
(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.
(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: 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.
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)
Depends on: 1103933
(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.
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)
Attached file pull-request (master)
Flags: needinfo?(wilsonpage)
Attachment #8535002 - Flags: review?(gmarty)
Attachment #8535002 - Flags: feedback?(gkrizsanits)
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+
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: