Open Bug 1103933 Opened 10 years ago Updated 2 years ago

RegisterElement should throw for protos that are already registered as interface prototype objects

Categories

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

defect

Tracking

()

People

(Reporter: gkrizsanits, Unassigned)

References

Details

Blocks: 1081037
The simplest way to do this is to have some sort of global (e.g. in the CC runtime) map of these objects.  The question is when to remove things from that map.  Perhaps what we really want are per-document maps (we have that already) and for the global map to have a counter; when documents die (to be defined) they reduce the counter in the global map and/or remove from it.

The big worry is defining "die", since we can have cycles through the global map...
(In reply to Please do not ask for reviews for a bit [:bz] from comment #1)
> The simplest way to do this is to have some sort of global (e.g. in the CC
> runtime) map of these objects.  The question is when to remove things from
> that map.  Perhaps what we really want are per-document maps (we have that
> already) and for the global map to have a counter; when documents die (to be
> defined) they reduce the counter in the global map and/or remove from it.
> 
> The big worry is defining "die", since we can have cycles through the global
> map...

The map what we have already handles cycles, so we should be fine with those.
How about making this global counter map a weakmap? I see no reason for it
to keep things alive.
> I see no reason for it to keep things alive.

Not only no reason, but we do not want it to keep anything alive.
Assignee: nobody → gkrizsanits
Ok, so since js weakmaps are not realistic to be used in C++ DOM code, nor adding a flag for all the js objects for this purpose... we should really define when to remove things from that global map as Boris said. Since this is all quite complex and magical and offers a wide range of opportunities to shoot ourself in the foot, I asked myself the question why do we need this at all? I mean what is the use of banning a proto to be used in multiple definitions on the first place? But even if we don't allow that why should we not allow the same proto to be used in different registries at the same time?
> I mean what is the use of banning a proto to be used in multiple definitions on the
> first place?

Registering a proto for a definition sets the .constructor property on the proto to the newly generated constructor.

Registering it multiple times would clobber this property with a different value each time, no?
I'm not sure I'm following completely, but would this still be a problem if we move to a custom elements without prototype hacks?
Why not?  Unclear.  In particular, if we move to passing in actual subclasses and not returning a new constructor, then there may not be an issue.

However, if the same constructor is registered for two different tag names, it then needs to take the tag name as an argument, right?  Otherwise, how would it call the superclass constructor?
(In reply to Boris Zbarsky [:bz] from comment #5)
> Registering a proto for a definition sets the .constructor property on the
> proto to the newly generated constructor.

Thanks, I looked over that fact for a moment. How about just throwing if the .constructor prop is set on the proto then? In fact we set the .constructor on the proto the first time we use it for a register call with non-configurable flag anyway, so this should not be a problem already, no? (getting late here sorry if I'm overlooking something again...)
> In fact we set the .constructor on the proto the first time we use it for a register call
> with non-configurable flag anyway,

I don't think we do.  You're thinking of the .prototype property on the ctor, which _is_ non-configurable and readonly.  But .constructor uses 0 as the flags to defineProperty in LinkConstructorAndPrototype.
(In reply to Boris Zbarsky [:bz] from comment #9)
> > In fact we set the .constructor on the proto the first time we use it for a register call
> > with non-configurable flag anyway,
> 
> I don't think we do.  You're thinking of the .prototype property on the
> ctor, which _is_ non-configurable and readonly.  But .constructor uses 0 as
> the flags to defineProperty in LinkConstructorAndPrototype.

Right. So we have a few options for tackling this issue, butI'm not too excited about either of them :( 

1. "Registering it multiple times would clobber this property with a different value each time"

(with some warning... not very elegant, could not argue why to make this change in the spec)

2. we make it non-configurable

(probably not what people would expect, there are some arguments to do it but it's not a conventional behavior, only primitives should have this behavior)

3., we do magic.

One thing I have in mind is splitting the check into two parts. For the trivial case, that the proto is registered in its own documents registry, we just use the existing map (actually we have to do some modifications there, but the point is that we hold these objects already by the registry)

For cross compartment registration we do some more tricks. When the registration happens, we have a cross-compartment-wrapper. Now we can either flag the wrapper itself, and then each time we get a proto from a different compartment, we check all it's ccw's if the flag is set. 
Or add a xraywrapper-expando-object like thing to ccw's (a storage object in the wrapped objects own compartment) and abuse a slot on that for storing flags.

4., comment 1 but then I have no idea about the consequences for GC/CC because if the JSObjects are the keys in the map we can do that only with strong referencing those objects, and I'm quite scared about how can this work out without leaking the world...
> only primitives should have this behavior

Even primitives don't have it.  Try this in your console:

  Number.prototype.constructor = "hey"; (2).constructor

XPConnect has several weak-ish maps already... We should ask Peter or Bobby how they suggest doing this.
(In reply to Boris Zbarsky [:bz] from comment #11)
> XPConnect has several weak-ish maps already... We should ask Peter or Bobby
> how they suggest doing this.

I'm afraid those are defined in js-land, but yeah let's do that.
Component: DOM → DOM: Core & HTML

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Assignee: gkrizsanits → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.