Bug 20567 - Change [[Prototype]] for concept-node-adopt?
Change [[Prototype]] for concept-node-adopt?
Status: NEW
Product: WebAppsWG
Classification: Unclassified
Component: DOM
unspecified
PC All
: P2 normal
: ---
Assigned To: Anne
public-webapps-bugzilla
:
Depends on: 24577 24579 27521
Blocks: 23810
  Show dependency treegraph
 
Reported: 2013-01-04 21:44 UTC by Anne
Modified: 2014-12-04 19:47 UTC (History)
19 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Travis Leithead [MSFT] 2013-01-21 19:29:32 UTC
IE9 added prototype re-parenting in order to ensure that document trees always had consistent prototype relationships. It's good for instanceof, and it's good for security defense-in-depth (so that some random node in a document doesn't provide an attack vector to some other frame's document after that document should no longer be available.).

A natural place for this is in:
"To adopt a node, with an ownerDocument, run these steps:"

But consideration also needs to be handled elsewhere for the document.open cases.
Comment 2 Anne 2013-01-22 11:09:14 UTC
Thanks Travis! Is the document.open() case the only other place in your codebase where this happens?

In the document.open() case, does this happen for non-Node objects as well (e.g. Array)?

Should this happen for DOMTokenList and similar such interfaces that are tied to nodes?

I'm adding public-script-coord@w3.org since they probably have an interest in this subject.
Comment 3 Travis Leithead [MSFT] 2013-01-23 23:36:52 UTC
Yes, in document.open we re-parent the window and document objects.

For node-tree appends cross-window we re-parent nodes.

We don't end-up re-parenting sub-objects like .style or the TokenList, etc. (if they were allocated from a previous window--these are usually on-demand allocated for JavaScript anyway).

This could be considered a bug.

We don't re-parent the JavaScript type system primitives (like Array) in the document.open scenario.
Comment 4 Cameron McCormack 2013-01-24 07:39:01 UTC
I've added a paragraph to #es-platform-objects explicitly stating that the associated global environment of a platform object can change, and when it does its [[Prototype]] is updated immediately.
Comment 5 Anne 2013-01-24 08:06:24 UTC
Travis, do you think it would be good to re-parent primitives?
Comment 6 Travis Leithead [MSFT] 2013-01-24 17:40:36 UTC
(In reply to comment #5)
> Travis, do you think it would be good to re-parent primitives?

I'm not sure. I suspect it would be a burden on our current implementation, and I'm not sure what kind of compatibility impact it might have. I'm leaning toward 'no' unless there's a compelling argument for doing so.
Comment 7 Boris Zbarsky 2013-01-24 20:41:06 UTC
Reparenting primitives sounds .... complicated.  I personally would rather not go there.
Comment 8 Anne 2013-01-24 20:53:47 UTC
(In reply to comment #3)
> Yes, in document.open we re-parent the window and document objects.

Travis, not the descendants of the Document object? What about Location, History, etc?
Comment 9 Maciej Stachowiak 2013-01-25 03:46:54 UTC
It seems to me like changing the prototypes of some objects (dom nodes) but not others (auxiliary objects connected to node, primitives), is more mysterious and confusing than not changing the prototypes at all.

Is there any web compatibility reason to modify node prototypes that would justify such a quirk?
Comment 10 Boris Zbarsky 2013-01-25 04:26:26 UTC
Maciej, you tell me.  Safari does the same thing.  Only in a gc-dependent way...
Comment 11 Geoffrey Garen 2013-01-25 05:53:57 UTC
I don't buy the instanceof argument.
    (a) Passing any object between windows breaks instanceof. I'm not convinced of the value of working around that in one special case.
    (b) Once you change from Window A's prototype to Window B's, instanceof in Window B will work but instanceof in Window A will now be broken.
    (c) node.onclick instanceof Function, and instanceof any custom properties, will still fail.
Comment 12 Geoffrey Garen 2013-01-25 05:55:20 UTC
In a way, I think this bug asks the wrong question. Our task isn't to pick a prototype -- it's to pick a set of observable properties. In a prototype-based language, the prototype is just one way to associate some properties with an object. 

This:

{ y: 0 }
   ^
{ x: 0 }
   ^
{  }

is logically equivalent to this:

{ x: 0, y: 0 }

If we want to logically sever the node's relationship to its old environment, and give it a new relationship to a new environment, we should be asking about all of its properties, and not just the ones belonging to its prototypes.

If that's the goal, I think the best solution is to create a second wrapper. Direct references to the old wrapper would continue to see that wrapper's identity and custom properties (including those implemented by its prototypes), but new references returned by the DOM (e.g. via getElementById) would use a new wrapper, with a new identity and a new set of custom properties defined by its new prototypes.

You might think that it's strange for a node's identity to change when it is adopted. But I think that's the most honest way to represent the operation you're describing. If adopting a node changes its very type, security identity, and so on, it's probably right to say that it's no longer equal to what it used to be.
Comment 13 Boris Zbarsky 2013-01-25 15:40:52 UTC
>    (a) Passing any object between windows breaks instanceof

That's being changed for nodes in WebIDL, note.  So the instanceof argument is moot anyway, for WebIDL objects.

> is logically equivalent to this:

It's trivially detectable to not be equivalent in all sorts of ways that the language provides.

Changing object identity on adopt is an interesting idea, but as I mentioned before not likely to be web-compatible.
Comment 14 Geoffrey Garen 2013-01-25 18:15:50 UTC
> >    (a) Passing any object between windows breaks instanceof
> 
> That's being changed for nodes in WebIDL, note.

Changed in what way?

>  So the instanceof argument
> is moot anyway, for WebIDL objects.

Anyway, I agree that instanceof should not be our primary motivation.

> Changing object identity on adopt is an interesting idea, but as I mentioned
> before not likely to be web-compatible.

This is WebKit's current behavior after garbage collection, which is frequent, and we have no compatibility bugs about it. So, that's one data point that this is web-compatible.
Comment 15 Boris Zbarsky 2013-01-25 18:55:01 UTC
> Changed in what way?

http://lists.w3.org/Archives/Public/public-script-coord/2013JanMar/0080.html

Or read the whole thread starting http://lists.w3.org/Archives/Public/public-script-coord/2013JanMar/0001.html if desired.  Note that it's the third time or so this has come up, and each time I've begged for a response from other UAs, with none from Apple thus far.

> This is WebKit's current behavior after garbage collection

Changing identity after garbage collection is by definition not web-detectable, no (as long as expandos don't get lost in the process, and ignoring this prototype issue)?  If the object was still pointed to by the web page, it wouldn't get collected.

I'm talking about cases in which foo.firstChild.parentNode == foo suddenly starts returning false, which is impossible right now but would no longer be impossible with your proposal.
Comment 16 Geoffrey Garen 2013-01-25 19:18:20 UTC
> Changing identity after garbage collection is by definition not
> web-detectable, no (as long as expandos don't get lost in the process, and
> ignoring this prototype issue)?

Ah, yes. In WebKit's current behavior, only the prototype change is observable.

> I'm talking about cases in which foo.firstChild.parentNode == foo suddenly
> starts returning false, which is impossible right now but would no longer be
> impossible with your proposal.

Got it.

If I have some time, maybe I can try out the full behavior in a WebKit nightly build.
Comment 17 Maciej Stachowiak 2013-01-25 20:03:04 UTC
(In reply to comment #10)
> Maciej, you tell me.

I don't know of any web compatibility reason to either modify or preserve the prototype of any objects transferred between documents.

>  Safari does the same thing.  Only in a gc-dependent way...

From what I can tell, the behavior proposed in this bug is also gc-dependent (for all non-Node objects).

(In reply to comment #13)
> >    (a) Passing any object between windows breaks instanceof
> 
> That's being changed for nodes in WebIDL, note.  So the instanceof argument
> is moot anyway, for WebIDL objects.

I don't see how WebIDL could possibly do that. Is it redefining the ECMAScript assignment operator, or ES function call semantics? It might be able to alter the case where a window directly retrieves nodes from another window's document but surely not any other form of "passing".
Comment 18 Boris Zbarsky 2013-01-25 20:09:32 UTC
Non-Node objects are an interesting question, yes.  It may be that the right thing is to reparent them in some cases if they will effectively get reparented by GC...

> I don't see how WebIDL could possibly do that.

Define what "foo instanceof SomeWebIDLObject" does, you mean?  It just can; ES6 is adding hooks for allowing the RHS to do whatever it wants to for instanceof.

So in particular, any element will test true for "instanceof anywindow.Element" with the proposed WebIDL changes.
Comment 19 Dimitri Glazkov 2013-07-19 20:02:33 UTC
So we have 3 choices:

1) Don't reparent the prototype, leaving node wrapper as-is.

Pros: the simplest, least-surprise behavior. Works exactly as any JS object.
Cons: provides opportunities for turning documents into a mish-mash from different scripting contexts and giant leaks.

2) Reparent the prototype, sort of doing the same thing WindowProxy does, but on a node

Pros: does its best at keeping the document prototype relationships consistent
Cons: contains dark magic of swapping prototypes and wrapper-proxying, magic that is not accessible to Muggles (web developers), still doesn't completely eliminate the problem, since we're not re-parenting built-ins' prototypes.

3) Change identity of the node.

Pros: the cleanest, Gordian solution to the problem of prototype consistency in the document.
Cons: may not be web-compatible.

Did I get this right?
Comment 20 Jonas Sicking 2013-07-19 21:14:17 UTC
On option 3 I would also add:

Cons: may be very surprising to developers since

elem.appendChild(x);
elem.removeChild(x);

will not always work.
Comment 21 Geoffrey Garen 2013-07-19 21:56:38 UTC
(In reply to comment #20)

Can you clarify under what conditions append followed by remove wouldn't work?
Comment 22 Dimitri Glazkov 2013-07-25 22:27:37 UTC
(In reply to comment #21)
> (In reply to comment #20)
> 
> Can you clarify under what conditions append followed by remove wouldn't
> work?

I think what he meant is that if "elem" is in Frame A, and x is in Frame B, appending "x" to "elem" will result in creation and insertion of a new element (let's call it "xA") as a child of "elem", so subsequent attempts to remove "x" from "elem" will do nothing, since "x" won't actually be a child of "elem".

Jonas, is this what you meant?
Comment 23 Geoffrey Garen 2013-07-26 00:26:30 UTC
> I think what he meant is that if "elem" is in Frame A, and x is in Frame B,
> appending "x" to "elem" will result in creation and insertion of a new
> element (let's call it "xA") as a child of "elem", so subsequent attempts to
> remove "x" from "elem" will do nothing, since "x" won't actually be a child
> of "elem".

OK, that's not the case -- at least not in the "new identity" scheme I proposed in Comment 12.

x and xA wrap the same underlying DOM node. Therefore elem.removeChild(x) succeeds.

The only thing that does not succeed is JavaScript identity comparison:

elem.insertBefore(x, elem.firstChild);
elem.firstChild == x; // Surprisingly false.

To me, this seems like the least bad consequence of the three options. Option 1's giant leaks are a show-stopper, in my opinion. And Option 2 is simply net more surprising to me than the identity change.

Easy to grok: Moving a node between windows changes its JavaScript identity.

Hard to grok: Moving a node between windows causes some of the node's JavaScript properties to disappear, while preserving some others, the exact behavior depending on the type of the node and/or the types of its properties.
Comment 24 Dimitri Glazkov 2013-07-31 22:42:09 UTC
(In reply to comment #23)
> > I think what he meant is that if "elem" is in Frame A, and x is in Frame B,
> > appending "x" to "elem" will result in creation and insertion of a new
> > element (let's call it "xA") as a child of "elem", so subsequent attempts to
> > remove "x" from "elem" will do nothing, since "x" won't actually be a child
> > of "elem".
> 
> OK, that's not the case -- at least not in the "new identity" scheme I
> proposed in Comment 12.
> 
> x and xA wrap the same underlying DOM node. Therefore elem.removeChild(x)
> succeeds.
> 
> The only thing that does not succeed is JavaScript identity comparison:
> 
> elem.insertBefore(x, elem.firstChild);
> elem.firstChild == x; // Surprisingly false.
> 
> To me, this seems like the least bad consequence of the three options.
> Option 1's giant leaks are a show-stopper, in my opinion. And Option 2 is
> simply net more surprising to me than the identity change.

Makes sense.

> Easy to grok: Moving a node between windows changes its JavaScript identity.
> 
> Hard to grok: Moving a node between windows causes some of the node's
> JavaScript properties to disappear, while preserving some others, the exact
> behavior depending on the type of the node and/or the types of its
> properties.

I suppose some surprise is to be expected in both option 2 and 3, since we're revealing that an object the user is holding onto is not a typical JS object. That is, unless the environment does exactly nothing (option 1), in which case the TC39 people are happy.

Travis, Jonas -- would love to hear your thoughts on option 2 as explained in comment 23.
Comment 25 Jonas Sicking 2013-08-01 00:07:28 UTC
I personally don't think it's at all easy to understand the concept of changing JS object identity.

It leads to all sorts of surprising and complicated issues.

Even the fact that you can have to JS objects that refer to the same underlying object is very complicated. We used to have that setup in our chrome code and it created lots of hard-to-understand situations for developers.

Basically any time you have code like "x == y" you are at risk of having a bug. We'd even have to bring back the Node.isSameNode(Node) function from DOM Core L3 in order to give people an opportunity to fix such code. But obviously most people will forget that the == operator no longer works.

In fact, we have the same issue in our C++ code since we can end up with objects having multiple implementations of the same interface. We very strongly try to avoid getting into that situation because it's really scary to have the '==' operator not working and to rely on that people use a function to check whether two objects are the same one.

I agree that the fact that some properties change and others don't is also confusing. Though in most cases properties will be replaced by new ones that have exactly the same name and behavior, so in most cases it is transparent.

And as far I can know we haven't had anyone actually complain about this happening in Gecko.
Comment 26 Erik Arvidsson 2013-08-06 20:05:01 UTC
Breaking object identity seems worse than changing the [[Prototype]]. Both are of course bad and I would prefer option 1. It is most consistent with other host objects such as Array, Date, RegExp, Map, Set etc.
Comment 27 Geoffrey Garen 2013-08-06 20:10:01 UTC
> I would prefer option 1. It is most consistent with
> other host objects such as Array, Date, RegExp, Map, Set etc.

I don't understand your meaning here. Array, Date, RegExp, Map, and Set do not change their prototypes when passed between windows.
Comment 28 Geoffrey Garen 2013-08-06 20:10:59 UTC
> I don't understand your meaning here. Array, Date, RegExp, Map, and Set do
> not change their prototypes when passed between windows.

Sorry, I get it: Option 1 keeps the prototype as-is.
Comment 29 Boris Zbarsky 2013-08-06 20:19:53 UTC
Option 1 requires that adopted nodes never have their wrappers collected, otherwise GC effects become observable (as in Safari; see http://lists.w3.org/Archives/Public/www-dom/2012OctDec/0149.html ).  That's not that great either.

It also means that it will become possible to test which nodes in a subtree have been touched from JS before by adopting the entire subtree into a different document and seeing which objects didn't change proto chains.  Not sure that's a huge problem in practice, but it introduces non-determinism that seems pretty unfortunate.
Comment 30 Bobby Holley (:bholley) 2013-08-21 21:29:53 UTC
(In reply to comment #27)
> Array, Date, RegExp, Map, and Set do
> not change their prototypes when passed between windows.

Well, they do the same as nodes when they're passed around. This issue only comes up with adoptNode, which is a very different beast, since it affects a DOM that generally lives in C++.

Option 1 seems pretty bad IMO. Speccing anything that makes GC behavior visible feels like a non-starter, and speccing something that requires UAs to leak like a sieve seems pretty bad as well. I don't think we should put vendors in the dilemma of correctly implementing a somewhat obscure spec requirement and huge memory wins on certain (potentially important) sites.

Option 2 seems like the best option to me. Maybe I've spent too much time working on the implementations here, but it doesn't seem all that surprising for the prototype to change when the node is adopted. Lots of other stuff (ownerDoc, parentNode, etc) change as well. WebComponents bindings are probably removed. After all, the node was adopted from one document to another, so it seems natural for the JS reflection to move from one global to another.
Comment 31 Dominic Cooney 2013-08-22 00:29:29 UTC
(In reply to comment #30)
> WebComponents bindings are
> probably removed. After all, the node was adopted from one document to
> another, so it seems natural for the JS reflection to move from one global
> to another.

Custom Elements don't change definitions when they are moved to a different document. We investigated doing that. Feedback from web developers was that keeping the original the Custom Element bindings when a Custom Element moved to a different document was preferable. I'm happy to provide details, but this bug thread probably isn't a good forum for that.
Comment 32 Anne 2013-10-05 19:38:42 UTC
I think there is an argument for changing prototypes. Adoption already re-parents, changes the node document, and does base URL change notification. So changing the prototype there to be consistent with the rest of the tree makes some amount of sense.

If you just move an element between globals it will behave just like Map, Set, etc. But if you adopt it, changes will be made.

So I guess I agree with Bobby.
Comment 33 Anne 2013-10-11 09:14:07 UTC
At some point we only discussed adopting nodes and stopped talking about window.open().

For adopting nodes we would need to change the node's inclusive descendants' __proto__ as well as the __proto__ of their associated objects, such as DOMTokenList and CSSStyleDeclaration.

The advantage here seems to be that we maintain consistency within the tree and reduce memory leaks.

For window.open() Bobby describes in http://lists.w3.org/Archives/Public/www-dom/2012OctDec/0173.html what we do in Gecko, which seems to allow for collecting the previous tree, but not necessarily the entire context. Is that correct?

If we still leak some of the old context it's hard to make a similar argument here.
Comment 34 Bobby Holley (:bholley) 2013-10-11 12:55:52 UTC
(In reply to Anne from comment #33)
 
> For window.open() Bobby describes in
> http://lists.w3.org/Archives/Public/www-dom/2012OctDec/0173.html what we do
> in Gecko, which seems to allow for collecting the previous tree, but not
> necessarily the entire context. Is that correct?

Mmm, not necessarily. Our setup has changed a bit since that posting, mainly because most everything is now on the new bindings, where we don't do orphan fixup.

For document.open, we basically just eject all the old nodes and leave them behind (orphaned) in the old window. Inserting them into the document again requires adopting them, just as we'd have if the nodes were non-orphan nodes that lived in some other document.

I would guess that, in 90% of cases, the old scope can simply die, because nobody is holding references to the old nodes. If they are holding references, those references will keep the old window alive.

> If we still leak some of the old context it's hard to make a similar
> argument here.

Per the above, I don't think they're really analogous. Moreover, the leak issue being discussed in this bug involves leaking the old window _even_ in the case when the node has been properly inserted into the new document (in the new window), just by virtue of the node's sordid past. This is very different from the document.domain case.
Comment 35 Geoffrey Garen 2013-10-14 17:56:28 UTC
> For adopting nodes we would need to change the node's inclusive descendants'
> __proto__ as well as the __proto__ of their associated objects, such as
> DOMTokenList and CSSStyleDeclaration.
> 
> The advantage here seems to be that we maintain consistency within the tree
> and reduce memory leaks.

What about event handlers? Event handlers will still reference their originating contexts through their scope chains, leaking the world.

I don't think we can change the scope chain of an event handler, since it's just a JavaScript function, and changing its scope chain will likely break its functionality.
Comment 36 Bobby Holley (:bholley) 2013-10-14 18:42:50 UTC
(In reply to Geoffrey Garen from comment #35)
> What about event handlers? Event handlers will still reference their
> originating contexts through their scope chains, leaking the world.
> 
> I don't think we can change the scope chain of an event handler, since it's
> just a JavaScript function, and changing its scope chain will likely break
> its functionality.

We can't move event handlers, that's for sure. But we could pretty easily spec that adopting a node clears all event handlers. Unless I'm missing something?

Even if we couldn't do that, we'd still be looking at a significant reduction in world-leaking, since most nodes won't have event handlers.
Comment 37 Dominic Cooney 2013-10-15 02:23:21 UTC
(In reply to Bobby Holley (:bholley) from comment #30)
> Option 2 seems like the best option to me. ... WebComponents bindings are
> probably removed. ...

I just wanted to chime in: WebComponents bindings *aren't* removed. To be specific, Custom Elements currently don't change prototype and their callbacks continue to fire when moved to a different document.

Although Custom Elements are nascent I think a resolution of this bug has to include a path for Custom Elements.

We're currently in the Option 1 world. I think Option 1 is preferable because it is simple; the handling of a node adopted into a different document behaves the same way as a node removed from a document but kept around with a reference from JavaScript. The downside is it is more leaky than other options. However other options aren't leak-free anyway, so I argue they're not worth the added complexity.

We (=Blink) tried mightily to implement something like Option 2 for Custom Elements. Ultimately it failed because the web developers we were talking to found it surprising that Custom Elements became neutered when adopted into a different document. In the implementation we got snared on brambles like when to deliver the leftView callback for a Custom Element in the process of changing prototypes. Explaining what is happening to the developer is super complex and implies extra callbacks or something. We couldn't figure it out.

I'm happy to go into details but maybe video chat with interested parties will be quicker than bug threads.
Comment 38 Bobby Holley (:bholley) 2013-10-15 10:26:06 UTC
(In reply to Dominic Cooney from comment #37)
> Although Custom Elements are nascent I think a resolution of this bug has to
> include a path for Custom Elements.

Agreed.
 
> We're currently in the Option 1 world. I think Option 1 is preferable
> because it is simple; the handling of a node adopted into a different
> document behaves the same way as a node removed from a document but kept
> around with a reference from JavaScript. The downside is it is more leaky
> than other options. However other options aren't leak-free anyway, so I
> argue they're not worth the added complexity.

I'm not sure I agree. The difference between leaking an entire scope in 10% of adoptNode and 100% of adoptNode calls matters a lot, at least in Gecko.
 
> We (=Blink) tried mightily to implement something like Option 2 for Custom
> Elements. Ultimately it failed because the web developers we were talking to
> found it surprising that Custom Elements became neutered when adopted into a
> different document.

Honestly, anything we do here is going to be surprising. And unfortunately, the surprise of "hey, I just lead a whole window" is much more subtle and harder to diagnose. Really I'd rather have crisp and visible behavior, even if surprising, than introduce an implicit requirement for the UA to do something pessimal from a performance standpoint.

> In the implementation we got snared on brambles like
> when to deliver the leftView callback for a Custom Element in the process of
> changing prototypes.

I don't understand why this is a problem. leftView fires when the node is removed from the document, which should occur before the adopt (which is what would trigger the reparent).

> Explaining what is happening to the developer is super
> complex and implies extra callbacks or something. We couldn't figure it out.

I would like to try to figure it out together. :-)
 
> I'm happy to go into details but maybe video chat with interested parties
> will be quicker than bug threads.

That's fine with me. Note that I'm in Europe until early November, so earlier in the day would be better. It would also be helpful to have Blake around, since I know next to nothing about Web Components. And Blake's been absent lately. I'll ping him to see if he's around.
Comment 39 Dominic Cooney 2013-10-16 00:48:03 UTC
(In reply to Bobby Holley (:bholley) from comment #38)
> (In reply to Dominic Cooney from comment #37)
> > Although Custom Elements are nascent I think a resolution of this bug has to
> > include a path for Custom Elements.
> 
> Agreed.

Great!

> > We're currently in the Option 1 world. I think Option 1 is preferable
> > because it is simple; the handling of a node adopted into a different
> > document behaves the same way as a node removed from a document but kept
> > around with a reference from JavaScript. The downside is it is more leaky
> > than other options. However other options aren't leak-free anyway, so I
> > argue they're not worth the added complexity.
> 
> I'm not sure I agree. The difference between leaking an entire scope in 10%
> of adoptNode and 100% of adoptNode calls matters a lot, at least in Gecko.

I'm not sure where the 10% and 100% numbers come from, but I assume you estimate 10% of elements to be Custom Elements at some point in the future?

I was referring to the fact that nodes removed from a document but not adopted into another document would leak. Does anyone have data?

> > In the implementation we got snared on brambles like
> > when to deliver the leftView callback for a Custom Element in the process of
> > changing prototypes.
> 
> I don't understand why this is a problem. leftView fires when the node is
> removed from the document, which should occur before the adopt (which is
> what would trigger the reparent).

Unfortunately not true because of the timing of Custom Element callbacks. We wanted to avoid the problems with Mutation Events running author script on surprising UA native callstacks, while giving authors apparently synchronous callbacks, so Custom Element callbacks are delayed until the UA is about to run script again. In cases like appendChild doing an implicit adoption, this means by the time you can run the leftView callback, the element is already in the new document. So when should its prototype change? etc. We found this to be a rabbit hole.

> > Explaining what is happening to the developer is super
> > complex and implies extra callbacks or something. We couldn't figure it out.
> 
> I would like to try to figure it out together. :-)

Sure. We hashed and thrashed this out on bug 20488 and ended up blocking on this one.

> > I'm happy to go into details but maybe video chat with interested parties
> > will be quicker than bug threads.
> 
> That's fine with me. Note that I'm in Europe until early November, so
> earlier in the day would be better. It would also be helpful to have Blake
> around, since I know next to nothing about Web Components. And Blake's been
> absent lately. I'll ping him to see if he's around.

I'm in JST but I'm happy to talk any time of the day or night.
Comment 40 Bobby Holley (:bholley) 2013-10-16 09:05:26 UTC
(In reply to Dominic Cooney from comment #39)
> > > include a path for Custom Elements.
> > Agreed.
> Great!

Just to be clear - this doesn't mean that I think that Custom Elements should drive what we do here irregardless of other considerations. It's possible that the Web Components spec will need to change. But I agree that we should at least have a plan in mind. :-)

> > I'm not sure I agree. The difference between leaking an entire scope in 10%
> > of adoptNode and 100% of adoptNode calls matters a lot, at least in Gecko.
> 
> I'm not sure where the 10% and 100% numbers come from, but I assume you
> estimate 10% of elements to be Custom Elements at some point in the future?
>
> I was referring to the fact that nodes removed from a document but not
> adopted into another document would leak.

I was talking about the case where document.open ejects all the nodes and leaves them behind in the old window, but still keeping a reference to the node in script. This leaks the old scope until the reference goes away. But my point is that this is a much narrower case than "every time a node is adopted cross-scope", and that matters a lot when we're talking about performance IMO. Also, there's disagreement about whether document.open is even in-scope for this bug.

> Does anyone have data?

I don't really know what meaningful data would look like on this. Also, this: http://bit.ly/17J9kc3 ;-)

> > I don't understand why this is a problem. leftView fires when the node is
> > removed from the document, which should occur before the adopt (which is
> > what would trigger the reparent).
> 
> Unfortunately not true because of the timing of Custom Element callbacks. We
> wanted to avoid the problems with Mutation Events running author script on
> surprising UA native callstacks, while giving authors apparently synchronous
> callbacks, so Custom Element callbacks are delayed until the UA is about to
> run script again.

I see. How about we just make adoptNode throw for any element with a binding? I don't imagine anyone would complain too hard about that.

In general, we may not be able to find a perfect solution that gives web developers the exact API that does everything they want. But I am strongly against speccing a requirement that we do something leaky in a situation that is quite common on the web.

> I'm in JST but I'm happy to talk any time of the day or night.

Ok. I'm quite flexible schedule-wise as well. Blake just emailed me saying he got back from vacation, and would weigh in on this thread soon.
Comment 41 Bobby Holley (:bholley) 2013-10-16 09:12:37 UTC
(In reply to Bobby Holley (:bholley) from comment #40)
> I see. How about we just make adoptNode throw for any element with a
> binding? I don't imagine anyone would complain too hard about that.

(And in the recursive case, it might arguably be better to just skip bound children than to cripple the whole abort. I'm just saying we should be creative and willing to say "we don't support that" when necessary, especially with nascent technology that doesn't carry a significant compatibility burden).
Comment 42 Dominic Cooney 2013-10-17 01:05:08 UTC
(In reply to Bobby Holley (:bholley) from comment #40)
> I was talking about the case where document.open ejects all the nodes and
> leaves them behind in the old window, but still keeping a reference to the
> node in script. This leaks the old scope until the reference goes away. But
> my point is that this is a much narrower case than "every time a node is
> adopted cross-scope", and that matters a lot when we're talking about
> performance IMO. Also, there's disagreement about whether document.open is
> even in-scope for this bug.

I'm arguing that while leaking is bad, there's a tradeoff involved here. And I don't know to what extent the cases discussed here cause leaks. Maybe time spent reducing leaks is better spent elsewhere.

> How about we just make adoptNode throw for any element with a
> binding? I don't imagine anyone would complain too hard about that.

Boris argues that throwing is a bad idea: <https://www.w3.org/Bugs/Public/show_bug.cgi?id=21485#c6> I agree.

re: Comment 41, compatibility is one constraint. I don't think the absence of compatibility constraints gives us a free pass on developer ergonomics.
Comment 43 Bobby Holley (:bholley) 2013-10-17 08:20:23 UTC
(In reply to Dominic Cooney from comment #42)
> And I don't know to what extent the cases discussed here cause leaks.

If we go with (1), then implementations will be required to keep the old global alive as long as the adopted node exists in a live document.

> Maybe time spent reducing leaks is better spent elsewhere.

I would much rather leave this prioritization to vendors than mandate it in the spec.
 
> > How about we just make adoptNode throw for any element with a
> > binding? I don't imagine anyone would complain too hard about that.
> 
> Boris argues that throwing is a bad idea:
> <https://www.w3.org/Bugs/Public/show_bug.cgi?id=21485#c6> I agree.

As partially noted in comment 41, I believe there are two approaches that would resolve Boris' worries about throwing midway through a recursive adopt:

(A) Don't throw when adopting ancestors of Custom Elements, but don't adopt the Custom Elements either. They get ejected from the document, but they (and their children) remain owned by the original document and don't appear in the subtree handed to the new document.

(B) Maintain a bit at each node indicating whether that node has Custom Elements in its descendants, and throw immediately when we try to adopt such a node. We'd only have to do work to compute this bit when applying/removing bindings and inserting/removing Custom Elements from the tree, neither of which are performance-critical operations. The algorithm would be O(TreeDepth*AverageChildCount), and very fast in practice.

> re: Comment 41, compatibility is one constraint. I don't think the absence
> of compatibility constraints gives us a free pass on developer ergonomics.

I am highly resistant to speccing memory leaks for the sake of developer ergonomics - it is very unlikely that developers will understand, care, or even notice the fact that adoptNode would increase memory footprint (not to mention all the sites that already use adoptNode). So they ship, and then users blame their browser for being bloated.

I feel strongly about this particular issue because the scope of the memory leaks (all cross-global adopts) is much wider than the scope of the feature and ergonomics they'd be supporting (leftView semantics for Custom Elements being adopted cross-global).
Comment 44 Dominic Cooney 2013-10-18 00:36:54 UTC
(In reply to Bobby Holley (:bholley) from comment #43)
> (In reply to Dominic Cooney from comment #42)
> > And I don't know to what extent the cases discussed here cause leaks.
> 
> If we go with (1), then implementations will be required to keep the old
> global alive as long as the adopted node exists in a live document.

I understand how the leak arises. I don't know how much it happens in practice.
 
> As partially noted in comment 41, I believe there are two approaches that
> would resolve Boris' worries about throwing midway through a recursive adopt:
> 
> (A) Don't throw when adopting ancestors of Custom Elements, but don't adopt
> the Custom Elements either. They get ejected from the document, but they
> (and their children) remain owned by the original document and don't appear
> in the subtree handed to the new document.
> 
> (B) Maintain a bit at each node indicating whether that node has Custom
> Elements in its descendants, and throw immediately when we try to adopt such
> a node. We'd only have to do work to compute this bit when applying/removing
> bindings and inserting/removing Custom Elements from the tree, neither of
> which are performance-critical operations. The algorithm would be
> O(TreeDepth*AverageChildCount), and very fast in practice.

It is very common to adopt subtrees containing Custom Elements across documents. A typical web component's UI is cloned out of a template (template document) and adopted into the custom element's shadow DOM (main document.) So I don't think throwing is feasible.

> > re: Comment 41, compatibility is one constraint. I don't think the absence
> > of compatibility constraints gives us a free pass on developer ergonomics.
> 
> I am highly resistant to speccing memory leaks for the sake of developer
> ergonomics - it is very unlikely that developers will understand, care, or
> even notice the fact that adoptNode would increase memory footprint (not to
> mention all the sites that already use adoptNode). So they ship, and then
> users blame their browser for being bloated.
> 
> I feel strongly about this particular issue because the scope of the memory
> leaks (all cross-global adopts) is much wider than the scope of the feature
> and ergonomics they'd be supporting (leftView semantics for Custom Elements
> being adopted cross-global).

I think we should look for a solution that doesn't involve a bad compromise. Basically, something that works for web developers and doesn't leak. I don't think these things are incompatible.
Comment 45 Blake Kaplan 2013-10-18 01:29:17 UTC
There are really no good solutions here.

Ignoring web components for a second, I have a slight preference for changing the prototype of nodes when they're adopted since that would mean that all nodes have the exact same interface (so if a page does, e.g. Element.prototype.operation = ()=>{...}, it can call operation() on any element in the page without worrying about whether or not 'operation' will be defined). The leak arguments are also pretty strong, IMO, as well as the fact that, at least in Gecko, any time we have a cross-global reference, we pay a performance penalty every time we access the object.

With web components, that gets even more complicated: removing the web component-added prototype is obviously a non-starter (we might as well detach the web component binding) and, given that, no matter what we do in this case will leak the "old" document/window. How important that is, I don't know.

I'm also happy to jump on a call to discuss this.
Comment 46 Scott Miles 2013-10-18 05:45:39 UTC
To be clear, I'm one of the guys pushing on Dominic to emphasize developer ergonomics, but I think he is drawing the right lines here.

I too am uncomfortable with failing zero-tolerance for memory leaks. 

But I believe it can be necessary for ergonomic concerns to trump leak robustness if the likelihood or total cost of the leaks, as a practical matter, is not significant.

One last thing: although it's tempting to try to omnibus improvements to document handling into this bug, I lean towards doing Occum's minimum from existing semantics. In other words, cross document node handling is already tricky, it may be better to fix that in isolation from Custom Elements.

Anyway, summary: Dominic has my proxy.
Comment 47 Bobby Holley (:bholley) 2013-10-18 08:16:02 UTC
(In reply to Dominic Cooney from comment #44)
> > As partially noted in comment 41, I believe there are two approaches that
> > would resolve Boris' worries about throwing midway through a recursive adopt:

> It is very common to adopt subtrees containing Custom Elements across
> documents. A typical web component's UI is cloned out of a template
> (template document) and adopted into the custom element's shadow DOM (main
> document.) So I don't think throwing is feasible.

Sorry - I meant that we'd throw only in the cases where the adopt crosses globals. Sorry if that was unclear.

> I think we should look for a solution that doesn't involve a bad compromise.
> Basically, something that works for web developers and doesn't leak. I don't
> think these things are incompatible.

I agree, though I believe that we have significantly more creative freedom with the Web Components spec than we do with concept-node-adopt. I don't see a way to avoid leaking if we do (1).

(In reply to Blake Kaplan from comment #45)
> With web components, that gets even more complicated: removing the web
> component-added prototype is obviously a non-starter (we might as well
> detach the web component binding) and, given that, no matter what we do in
> this case will leak the "old" document/window.

This is why I think we shouldn't allow script to adopt bound nodes across globals. The binding is tied to the document, so if you're using the binding, you're tied to the document. If we do this, we won't leak.
 
> I'm also happy to jump on a call to discuss this.

You're in CA right now, right? That makes the three of us perfectly geo-distributed at 8-hour intervals around the globe. This isn't insurmountable, but I think we should continue async as long as it's working. ;-)

(In reply to Scott Miles from comment #46)
> But I believe it can be necessary for ergonomic concerns to trump leak
> robustness if the likelihood or total cost of the leaks, as a practical
> matter, is not significant.

If this were only about Custom Elements, I'd be less worried. But what we decide here affects every cross-global adopt.

> One last thing: although it's tempting to try to omnibus improvements to
> document handling into this bug, I lean towards doing Occum's minimum from
> existing semantics. In other words, cross document node handling is already
> tricky, it may be better to fix that in isolation from Custom Elements.

I agree 100%. This is why I think we should just punt for cross-global adopts of Custom Elements (by speccing that it throws or fails somehow either (A) or (B) from comment 43 is fine by me). We can always revisit that later and make it not throw if we come up with some idea of meaningful cross-global semantics for Custom Elements.
Comment 48 Erik Arvidsson 2013-10-18 14:24:56 UTC
My main concerns about changing the [[Prototype]] on adopt is complexity and consistency.

Complexity: We now need to enumerate all the properties of which we need to change their values. What if this object is shared between different realms? For example I have a two elements using the same value for their onclick webidl attribute. Am I supposed to change the prototype of it?

Consistency: This only happens for nodes that are adopted. What about other cross realm objects. What if I reference an object across the boundary without adopting it (maybe it cannot be adopted).

I think playing the memory leak card is distraction. If you have multiple realms that can reference each other there will be cross realm references. Both the GC and the app author needs to be aware of this so that they do not hold on to cross realm references longer than needed. Changing the [[Prototype]] on adopt is not going to make this problem go away. It might alleviate the problem a bit but I've yet to see any number on how much it will help in reality. People could always use importNode if they needed to get a new fresh clone in the new document. Better tools would probably be a better bang for the buck than changing the [[Prototype]].
Comment 49 Bobby Holley (:bholley) 2013-10-18 15:46:46 UTC
(In reply to Erik Arvidsson from comment #48)
> Complexity: We now need to enumerate all the properties of which we need to
> change their values. What if this object is shared between different realms?
> For example I have a two elements using the same value for their onclick
> webidl attribute. Am I supposed to change the prototype of it?

No, I think the handler should not be adopted. Per comment 36 I think we can probably just clear event handlers and listeners on cross-global adopt.

In general, the I think that 'owned' things (like .style - a short list, I'd think) should move, whereas 'unowned' things (like event handlers) should not. Though given that (per comment 3) IE doesn't currently preserve .style identity across reparents, we probably have a lot of flexibility in what we do here.

> Consistency: This only happens for nodes that are adopted. What about other
> cross realm objects. What if I reference an object across the boundary
> without adopting it (maybe it cannot be adopted).

They're pretty distinct concepts, IMO. In one case, we're just referencing something that clearly belongs somewhere else. In the other case, we're actually changing ownership (visible in that ownerDocument changes).
 
> I think playing the memory leak card is distraction. If you have multiple
> realms that can reference each other there will be cross realm references.
> Both the GC and the app author needs to be aware of this so that they do not
> hold on to cross realm references longer than needed.

I think the semantics are pretty different. In one case, you're holding an object alive by virtue of holding a live reference to it. Everybody is well aware that this affects what the GC is able to collect.

In other other case, the object holds an entire realm alive just by virtue of sitting in a document, even after it has supposedly been "adopted".

> Changing the
> [[Prototype]] on adopt is not going to make this problem go away. It might
> alleviate the problem a bit but I've yet to see any number on how much it
> will help in reality. People could always use importNode if they needed to
> get a new fresh clone in the new document.

I'm not saying we should encourage cross-scope adopts - I'm just concerned that there are already a fair number of them out on the web.

> Better tools would probably be a
> better bang for the buck than changing the [[Prototype]].

Well, it depends whose buck. ;-) Mozilla and Microsoft already have this machinery, so specing things the other way requires effort on the other side. And at least for Gecko, it would be pretty non-trivial due to the changes in invariants - probably equivalent to the amount of effort to implement prototype munging in Blink.

In general, (2) seems like the "right" way to spec this if it's a feature that we care to support, both from semantic and performance standpoints. The only argument against the leak issue is "maybe people don't use this feature very often".

I wouldn't be comfortable doing (1) unless we emitted a console warning and officially deprecated cross-global adopts. I can run some Telemetry numbers to see how much this happens on the web today, but only if the spec community is actually gung-ho about such a deprecation effort.
Comment 50 Boris Zbarsky 2013-10-18 16:15:22 UTC
> What about event handlers?

What Gecko does is that when a node is adopted it goes through its event handler content attributes and recompiles those to event handlers, blowing away whatever was there at the time.  This makes sure that event handlers compiled from content attributes have the right owner document on the scope chain, but leads to effects like this:

<div id="x" onclick="alert('hi');">Click me</div>
<iframe></iframe>
<script>
  window.onload = function() {
    var d = document.getElementById("x");
    d.onclick = function() { alert('bye'); }
    frames[0].document.body.appendChild(d);
  }
</script>

where clicking the div in the iframe will alert 'hi'.  In practice, I suspect no one ever mixes event handler attributes and explicit assignment of event handlers like this.

If there is no event handler content attribute set on the element for a given event handler, it's left as-is.

> I was referring to the fact that nodes removed from a document but not
> adopted into another document would leak.

Only if something is still holding explicit JS references to them.  Otherwise they just get GCed.
Comment 51 Dimitri Glazkov 2013-10-18 16:31:05 UTC
Thank you for working on this problem! I salute you all and cherish the thoughtful and civil discussion. Whatever solution you nice folks arrive at, here's my plea to not make custom elements a special case, compared to HTML elements. That would block the path toward explaining the magic of HTML elements in terms of custom element machinery, and I think that would be bad.
Comment 52 Bobby Holley (:bholley) 2013-10-18 18:22:45 UTC
(In reply to Dimitri Glazkov from comment #51)
> Thank you for working on this problem! I salute you all and cherish the
> thoughtful and civil discussion.

:-)

> Whatever solution you nice folks arrive at,
> here's my plea to not make custom elements a special case, compared to HTML
> elements. That would block the path toward explaining the magic of HTML
> elements in terms of custom element machinery, and I think that would be bad.

Are you talking about stuff like <marquee>? If so, I don't think it's a major problem. We can just define that stuff every-so-slightly differently as a "Platform Custom Element" or something, which don't fire leftView. The binding is available in every document, so we could just fix up the prototypes like we do for every other node. Unless there's a bug, this should already be the case with Gecko's XBL-implemented marquee.
Comment 53 Dimitri Glazkov 2013-10-18 19:23:12 UTC
(In reply to Bobby Holley (:bholley) from comment #52)
> (In reply to Dimitri Glazkov from comment #51)
> > Thank you for working on this problem! I salute you all and cherish the
> > thoughtful and civil discussion.
> 
> :-)
> 
> > Whatever solution you nice folks arrive at,
> > here's my plea to not make custom elements a special case, compared to HTML
> > elements. That would block the path toward explaining the magic of HTML
> > elements in terms of custom element machinery, and I think that would be bad.
> 
> Are you talking about stuff like <marquee>? If so, I don't think it's a
> major problem. We can just define that stuff every-so-slightly differently
> as a "Platform Custom Element" or something, which don't fire leftView.

This is sort of what I am hoping to avoid. Ideally, "Platform custom elements" are only different in that they don't have to have dashes in their tag names.
Comment 54 Bobby Holley (:bholley) 2013-10-18 20:12:41 UTC
(In reply to Dimitri Glazkov from comment #53)
> > Are you talking about stuff like <marquee>? If so, I don't think it's a
> > major problem. We can just define that stuff every-so-slightly differently
> > as a "Platform Custom Element" or something, which don't fire leftView.
> 
> This is sort of what I am hoping to avoid. Ideally, "Platform custom
> elements" are only different in that they don't have to have dashes in their
> tag names.

In light of the other considerations on the table, this doesn't seem super compelling. There are already subtle differences (the lack of an 'x', the fact that the binding is UA-supplied rather than script-supplied, etc). Stating that they don't fire leftView doesn't seem all that problematic.
Comment 55 Bobby Holley (:bholley) 2013-10-18 20:24:42 UTC
(An alternative to all of this, of course, is to revisit the Web Components spec and find a way to make Custom Elements adopt properly across globals, with possible revisions to the callback setup to make everything sane).
Comment 56 Bobby Holley (:bholley) 2013-10-30 12:22:25 UTC
I've been running some Telemetry in Gecko to measure this stuff in the wild:

https://bugzilla.mozilla.org/show_bug.cgi?id=928476#c14

The numbers are still rough (since they reflect the workload of bleeding edge Nightly users), but so far they indicate (with lots of statisticians' asterisks) that (1) would cause us to leak an average of 2.5 globals at any given time.

The data is certainly incomplete, and we could get more if people want, but if the numbers remain in this ballpark, I'm not comfortable speccing (1).

IMO, the proper way forward here is to spec (2) and figure out a way to handle this properly in the Web Components spec while it's still young.

What are other folks' thoughts?
Comment 57 Dominic Cooney 2013-11-01 00:43:39 UTC
(In reply to Bobby Holley (:bholley) from comment #56)
> I've been running some Telemetry in Gecko to measure this stuff in the wild:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=928476#c14
> 
> The numbers are still rough (since they reflect the workload of bleeding
> edge Nightly users), but so far they indicate (with lots of statisticians'
> asterisks) that (1) would cause us to leak an average of 2.5 globals at any
> given time.

What's the units of this measurement? 2.5 globals per... ?

> The data is certainly incomplete, and we could get more if people want, but
> if the numbers remain in this ballpark, I'm not comfortable speccing (1).
> 
> IMO, the proper way forward here is to spec (2) and figure out a way to
> handle this properly in the Web Components spec while it's still young.
> 
> What are other folks' thoughts?

I will try to take another stab at this. Let me talk to my web developer brain trust. Apart from "no leaks", are there any other constraints a solution needs to incorporate?
Comment 58 Bobby Holley (:bholley) 2013-11-01 12:28:40 UTC
(In reply to Dominic Cooney from comment #57)
> What's the units of this measurement? 2.5 globals per... ?

Per running instance of Firefox. We maintain a count of live Windows whose Documents have adopted at least one node from another Window. During every GC, telemetry-enabled Firefox builds record this value as a data point in the telemetry report, which gets sent to Mozilla's servers every 24 hours or so. So it's an average over time, users, workloads, browsing patterns, etc.

> I will try to take another stab at this. Let me talk to my web developer
> brain trust. Apart from "no leaks", are there any other constraints a
> solution needs to incorporate?

Well, I'm pretty sure to get "no leaks" we need to spec (2) for the general DOM case (unless there's another solution that has yet to be raised). So the main question is how Web Components wants to treat itself in this whole charade.

I think the ideal thing would be to treat it as a first-class citizen and find a way to make it work right cross-global. This involves structuring the callbacks to take this into a account, and figuring out a way to either re-create the binding in the new scope, or remove it gracefully.

The easier option is just to spec that these things can't be adopted, and decide where/how to fail when people try. But it's not a very forward-thinking strategy if we really believe in Web Components as a foundational technology for the future web.

I'm certainly no expert in Web Components or webdev ergonomics, but I'd nevertheless enjoy being CCed for these discussions to gain a fuller understanding of the issues at play here. :-)
Comment 59 Bobby Holley (:bholley) 2014-01-14 00:14:38 UTC
Dominic - any updates with respect to comment 57? I'm meeting up with anne, heycam, and bz in two weeks, and it's likely that we'll do some talking about this.

(I can't remember if you're at Google SF, but if you are, we'll be just around the corner, and should meet up).
Comment 60 Dominic Cooney 2014-01-14 01:43:16 UTC
(In reply to Bobby Holley (:bholley) from comment #59)
> Dominic - any updates with respect to comment 57? I'm meeting up with anne,
> heycam, and bz in two weeks, and it's likely that we'll do some talking
> about this.
> 
> (I can't remember if you're at Google SF, but if you are, we'll be just
> around the corner, and should meet up).

Great! I'm in Tokyo. No travel planned. I might be able to dig up another Googler for you though. If you can forward notes to a list that would be great.

re: Web Components, I think this issue specifically affects Custom Elements. I believe any interaction with Shadow DOM and HTML Imports will be the same as an interaction with an ordinary node (ShadowRoot or LINK.)

Custom Elements are special because they have an author-supplied prototype and callbacks. This is pretty analogous to having an event handler, so maybe whatever solution works for event handlers could be adapted to apply to Custom Elements?

Not sure if this interests you but I can tell you about the status quo with Custom Elements in Chrome:

Chrome doesn't swizzle the prototype when a node is adopted (not Custom Elements nor any other node.) But we also don't want to leak, so a Custom Element's callbacks only work as long as the context it was registered in is alive.

This means if you have an IFRAME, register X-A in there, and move an X-A into the parent frame's document its callbacks will keep working provided the frame is alive. If you remove the frame, the callbacks stop firing (and incidentally we free the IFRAME, its document, etc.) The element's prototype still points to the author-supplied one, though.

So far I've had one bug report about this behavior. The author expected their callbacks to continue firing indefinitely.

I think the idea of a callback that lets the author switch an element's definition sounds promising. In the aforementioned author's case they're embedding Blink so they can exercise a degree of omnipotency the web author can't. Making this work for the web author sounds tricky... is there a proposal somewhere?
Comment 61 Bobby Holley (:bholley) 2014-01-16 00:31:05 UTC
(In reply to Dominic Cooney from comment #60)

> Custom Elements are special because they have an author-supplied prototype
> and callbacks. This is pretty analogous to having an event handler, so maybe
> whatever solution works for event handlers could be adapted to apply to
> Custom Elements?

That seems reasonable, yes. But what about the issue you mentioned in comment 37 (the timing for firing detachedCallback)?

> Not sure if this interests you but I can tell you about the status quo with
> Custom Elements in Chrome:
> 
> Chrome doesn't swizzle the prototype when a node is adopted (not Custom
> Elements nor any other node.)

Right. We're trying to spec the swizzling (or lack thereof, though I would non-neutrally contend that this side is losing in light of the leak data).

> But we also don't want to leak, so a Custom
> Element's callbacks only work as long as the context it was registered in is
> alive.
> 
> This means if you have an IFRAME, register X-A in there, and move an X-A
> into the parent frame's document its callbacks will keep working provided
> the frame is alive. If you remove the frame, the callbacks stop firing (and
> incidentally we free the IFRAME, its document, etc.) The element's prototype
> still points to the author-supplied one, though.

But doesn't the prototype live in the window in which it was registered? How can you free the IFRAME if the element's prototype still points to an object in its scope?

> I think the idea of a callback that lets the author switch an element's
> definition sounds promising.

Can you explain this more?
Comment 62 Dominic Cooney 2014-01-16 01:54:42 UTC
(In reply to Bobby Holley (:bholley) from comment #61)
> (In reply to Dominic Cooney from comment #60)
> 
> > Custom Elements are special because they have an author-supplied prototype
> > and callbacks. This is pretty analogous to having an event handler, so maybe
> > whatever solution works for event handlers could be adapted to apply to
> > Custom Elements?
> 
> That seems reasonable, yes. But what about the issue you mentioned in
> comment 37 (the timing for firing detachedCallback)?

By not changing the prototype the timing of the detachedCallback is a non-issue, because there's only one set of callbacks. So they're consistent with their prototype. This problem arises if you try to change the prototype and callbacks.

(Presaging your question about an event for this reparenting...) I was speculating that if there was a callback to say, hey, this element is switching documents so authors can do what they want (switch definitions, drop definitions, leak definitions) and are flexible about the timing of that (maybe the prototype swizzling happens slightly after the element is in the new document) that might be a workable solution. The problem is I'm not sure what authors need here.

> > Not sure if this interests you but I can tell you about the status quo with
> > Custom Elements in Chrome:
> > 
> > Chrome doesn't swizzle the prototype when a node is adopted (not Custom
> > Elements nor any other node.)
> 
> Right. We're trying to spec the swizzling (or lack thereof, though I would
> non-neutrally contend that this side is losing in light of the leak data).

I'm not sure I'd frame it in terms of winning and losing... UAs today behave differently in this area, whether by design or historical accident, and being compatible would be good. But if we can't come up with something workable for UAs then probably the incompatibility will continue to exist.

> > But we also don't want to leak, so a Custom
> > Element's callbacks only work as long as the context it was registered in is
> > alive.
> > 
> > This means if you have an IFRAME, register X-A in there, and move an X-A
> > into the parent frame's document its callbacks will keep working provided
> > the frame is alive. If you remove the frame, the callbacks stop firing (and
> > incidentally we free the IFRAME, its document, etc.) The element's prototype
> > still points to the author-supplied one, though.
> 
> But doesn't the prototype live in the window in which it was registered? How
> can you free the IFRAME if the element's prototype still points to an object
> in its scope?

Right, if something like that exists, the object would leak.
Comment 63 Boris Zbarsky 2014-01-16 02:08:51 UTC
Prototypes generally reference their global in all sorts of ways; I'd be pretty surprised if you can keep the proto alive but not the global...
Comment 64 Dominic Cooney 2014-01-16 02:42:25 UTC
(In reply to Boris Zbarsky from comment #63)
> Prototypes generally reference their global in all sorts of ways; I'd be
> pretty surprised if you can keep the proto alive but not the global...

Yes, you're right. I misremembered some of our leak tests. But those are for navigation, etc. and not node adoption.
Comment 65 Bobby Holley (:bholley) 2014-01-16 02:56:15 UTC
(In reply to Dominic Cooney from comment #62)

> (Presaging your question about an event for this reparenting...) I was
> speculating that if there was a callback to say, hey, this element is
> switching documents so authors can do what they want (switch definitions,
> drop definitions, leak definitions) and are flexible about the timing of
> that (maybe the prototype swizzling happens slightly after the element is in
> the new document) that might be a workable solution. The problem is I'm not
> sure what authors need here.

Would detachedCallback fire in that case?

I'm certainly happy to add whatever callbacks we need here. But IIUC, the issue is less that authors want to be notified about adoption, and more that state during the detachedCallback in that case is confusing. We could certainly say "if you want to be sure to hear when your document is removed from the DOM, you need to have both a detachedCallback and an adoptedCallback". But that's probably pretty annoying, and most developers will probably just skip it, and get confused when their element disappears via an adopt.

> > Right. We're trying to spec the swizzling (or lack thereof, though I would
> > non-neutrally contend that this side is losing in light of the leak data).
> 
> I'm not sure I'd frame it in terms of winning and losing... UAs today behave
> differently in this area, whether by design or historical accident, and
> being compatible would be good. But if we can't come up with something
> workable for UAs then probably the incompatibility will continue to exist.

I don't think we're seeing eye to eye here. Here's how I read the sequence of events in this bug:

(1) Anne proposes speccing prototype swizzling for concept-adopt-node (applying to all DOM nodes, not just custom elements). Discussion ensues. bholley makes a lot of noise about leaks.

(2) In comment 37, Domenic requests that we consider Web Components as a stakeholder in this discussion. He reports that Web Components had trouble defining the callbacks semantics for adoptNode, and punted on prototype swizzling.

(3) bholley proposes special-casing Web Components by making it illegal to adopt them.

(4) In comment 51, Dmitri expresses his dismay with (3), and says that we should make Web Components behave as much like the rest of the DOM as possible.

(5) bholley provides Telemetry data indicating that prototype swizzling is required in the general case to prevent unacceptably-large leaks.

(6) In comment 57, Domenic signals his intention to go back to work on the Web Components spec to come up with a solution for custom elements.

Does this match your interpretation? If not, what am I missing?

> > But doesn't the prototype live in the window in which it was registered? How
> > can you free the IFRAME if the element's prototype still points to an object
> > in its scope?
> 
> Right, if something like that exists, the object would leak.

This is what I want to avoid, at least for non Custom Elements. In this sentence, I read "if something like that exists" as "if a custom element is adopted cross-global". Is that correct?


In the light of comment 56, I would wager that it is more likely than not that swizzling will get specced for the DOM in general. If that does happen, the options I can think of for Custom Elements are:
(A) Swizzle the prototypes. If a Custom Element with the same name been registered the new scope, the prototype points to that. Otherwise, the element loses its magic. This approach requires figuring out the semantics of |detachedCallback|.
(B) Leave the prototype in the old scope for Custom Elements. This leaks, and makes Custom Elements more special, but might be more friendly to web developers.
(C) Forbid adoption. This is easy, but probably makes Custom Elements too special to re-implement existing things like marquee.

I think (A) is the best option if we want Custom Elements to be first-class citizens.
Comment 66 Dominic Cooney 2014-01-17 00:35:47 UTC
(In reply to Bobby Holley (:bholley) from comment #65)
> (In reply to Dominic Cooney from comment #62)
> 
> > (Presaging your question about an event for this reparenting...) I was
> > speculating that if there was a callback to say, hey, this element is
> > switching documents so authors can do what they want (switch definitions,
> > drop definitions, leak definitions) and are flexible about the timing of
> > that (maybe the prototype swizzling happens slightly after the element is in
> > the new document) that might be a workable solution. The problem is I'm not
> > sure what authors need here.
> 
> Would detachedCallback fire in that case?

Probably, but I don't know.

> I'm certainly happy to add whatever callbacks we need here. But IIUC, the
> issue is less that authors want to be notified about adoption, and more that
> state during the detachedCallback in that case is confusing. We could
> certainly say "if you want to be sure to hear when your document is removed
> from the DOM, you need to have both a detachedCallback and an
> adoptedCallback". But that's probably pretty annoying, and most developers
> will probably just skip it, and get confused when their element disappears
> via an adopt.

Probably. There's no element death callback, so maybe being adopted away is like that?

> > > Right. We're trying to spec the swizzling (or lack thereof, though I would
> > > non-neutrally contend that this side is losing in light of the leak data).
> > 
> > I'm not sure I'd frame it in terms of winning and losing... UAs today behave
> > differently in this area, whether by design or historical accident, and
> > being compatible would be good. But if we can't come up with something
> > workable for UAs then probably the incompatibility will continue to exist.
> 
> I don't think we're seeing eye to eye here. Here's how I read the sequence
> of events in this bug:
> 
> (1) Anne proposes speccing prototype swizzling for concept-adopt-node
> (applying to all DOM nodes, not just custom elements). Discussion ensues.
> bholley makes a lot of noise about leaks.
> 
> (2) In comment 37, Domenic requests that we consider Web Components as a
> stakeholder in this discussion. He reports that Web Components had trouble
> defining the callbacks semantics for adoptNode, and punted on prototype
> swizzling.
> 
> (3) bholley proposes special-casing Web Components by making it illegal to
> adopt them.
> 
> (4) In comment 51, Dmitri expresses his dismay with (3), and says that we
> should make Web Components behave as much like the rest of the DOM as
> possible.

FWIW I agree with Dimitri. I think it's weird if you can't adopt Custom Elements like any other kind of element.

> (5) bholley provides Telemetry data indicating that prototype swizzling is
> required in the general case to prevent unacceptably-large leaks.

I don't think this quite follows... I saw data about leak frequency, but not size. But I'm with you that all things being equal it is better not to leak.

> (6) In comment 57, Domenic signals his intention to go back to work on the
> Web Components spec to come up with a solution for custom elements.
> 
> Does this match your interpretation? If not, what am I missing?

Modulo the inline comments that's it. I wasn't able to come up with something better. I think developers on Chrome are happy their Custom Elements continue to work in other documents--even that is probably a rare case--and they always bear the cost of leaks in general because Chrome doesn't do any prototype resetting.

> > > But doesn't the prototype live in the window in which it was registered? How
> > > can you free the IFRAME if the element's prototype still points to an object
> > > in its scope?
> > 
> > Right, if something like that exists, the object would leak.
> 
> This is what I want to avoid, at least for non Custom Elements. In this
> sentence, I read "if something like that exists" as "if a custom element is
> adopted cross-global". Is that correct?

In the status quo, that's correct.

Do you have any data to share about the size of the JavaScript state versus the DOM state?

> In the light of comment 56, I would wager that it is more likely than not
> that swizzling will get specced for the DOM in general. If that does happen,
> the options I can think of for Custom Elements are:
> (A) Swizzle the prototypes. If a Custom Element with the same name been
> registered the new scope, the prototype points to that. Otherwise, the
> element loses its magic. This approach requires figuring out the semantics
> of |detachedCallback|.

This is basically right. You're proposing a relatively specific solution here--that if the names are equal, then one definition should be substituted for another. There might be specific tweaks to a solution in this area that makes authors happier.

> (B) Leave the prototype in the old scope for Custom Elements. This leaks,
> and makes Custom Elements more special, but might be more friendly to web
> developers.

It would be nice to not add more magic (although it's only magic if other nodes behavior changes... FWIW this matches most closely what Chrome does today.)

> (C) Forbid adoption. This is easy, but probably makes Custom Elements too
> special to re-implement existing things like marquee.

This is what Dimitri was arguing against as you noted above, etc.

> I think (A) is the best option if we want Custom Elements to be first-class
> citizens.
Comment 67 Bobby Holley (:bholley) 2014-01-17 02:00:55 UTC
(In reply to Dominic Cooney from comment #66)
> Probably. There's no element death callback, so maybe being adopted away is
> like that?

By element death do you mean the destruction of the underlying element? That only happens when (a) the node (or its ancestor) is detached from the document or (b) the entire window is destroyed, right?

> > (4) In comment 51, Dmitri expresses his dismay with (3), and says that we
> > should make Web Components behave as much like the rest of the DOM as
> > possible.
> 
> FWIW I agree with Dimitri. I think it's weird if you can't adopt Custom
> Elements like any other kind of element.

I'm inclined to agree, from an author's perspective. But we also don't want to offer people APIs that turn out to be perf/memory gotchas. So I think we should design things so that this is safe to do.
 
> > (5) bholley provides Telemetry data indicating that prototype swizzling is
> > required in the general case to prevent unacceptably-large leaks.
> 
> I don't think this quite follows... I saw data about leak frequency, but not
> size.

Well, it's the size of a javascript global scope, which will vary between implementations. But it's significant. Looking at the tabs I have open right now, this would be (in Firefox nightly):
example.com: 200k
html5 spec: 6MB
gmail: 22MB
Wunderlist: 15MB
Google spreadsheets: 12MB

So if we leak, on average, 2 instances of example.com at any given time, it's probably not such a big deal. But if we leak 2 instances of Wunderlist, it's a really big deal, especially on mobile.

> Modulo the inline comments that's it. I wasn't able to come up with
> something better. I think developers on Chrome are happy their Custom
> Elements continue to work in other documents--even that is probably a rare
> case--

Yeah, authors don't generally care much about leaks. Users just blame the browser. :-(


> Do you have any data to share about the size of the JavaScript state versus
> the DOM state?

in Gmail, Wunderlist, and example.com, the JS state dwarfs the DOM state by an order of magnitude. It's about 5x for spreadsheets. It's the opposite story for the HTML5 spec, though that always tends to be a bit of an outlier.

> > In the light of comment 56, I would wager that it is more likely than not
> > that swizzling will get specced for the DOM in general. If that does happen,
> > the options I can think of for Custom Elements are:
> > (A) Swizzle the prototypes. If a Custom Element with the same name been
> > registered the new scope, the prototype points to that. Otherwise, the
> > element loses its magic. This approach requires figuring out the semantics
> > of |detachedCallback|.
> 
> This is basically right. You're proposing a relatively specific solution
> here--that if the names are equal, then one definition should be substituted
> for another. There might be specific tweaks to a solution in this area that
> makes authors happier.

Totally. This is what I'm hoping you can figure out!
 
> > (B) Leave the prototype in the old scope for Custom Elements. This leaks,
> > and makes Custom Elements more special, but might be more friendly to web
> > developers.
> 
> It would be nice to not add more magic (although it's only magic if other
> nodes behavior changes... FWIW this matches most closely what Chrome does
> today.)

Right.

My big concern here is that what we do now for Web Components is going to constrain us in the future. For regular HTML elements, web authors aren't depending on the existence/non-existence of swizzling, because it differs between browsers, and the prototype's global generally isn't all that important. But if we let authors depend on no-swizzle for Web Components now, we'll probably have to preserve that behavior forever, even if Blink ends up aligning with Gecko and Trident and implementing swizzling for non Custom Elements.

This is why I think we should implement behavior now that is at least compatible with swizzling should we decide to do it. Hopefully we can engineer something clever. If not, I think we should at least throw, so that we have the flexibility to tackle the problem later without breaking the Web.
Comment 68 Anne 2014-01-28 22:49:01 UTC
We had a discussion within the Gecko team about this. We agreed on these things:

1. We have to deal with multiple globals that can access each other. (ES6 provides constructors, <iframe> already exists.)
2. We would like to reduce memory leaks.
3. We would like a consistent story across custom elements and normal elements, given that the latter might end up being implemented using the former.

Given that we do not want to repeat mutation events, a proposal was made to queue a microtask during adopt that would be responsible for the clean up. In case of custom elements the microtask would run a callback and the developer of the custom element would be responsible for the cleanup.

This makes it deterministic when prototype cleanup happens and consistent across normal and custom elements.
Comment 69 Dimitri Glazkov 2014-02-06 20:07:31 UTC
Just a quick update: a bunch of Mozilla and Google folk chewed some more on the problem and reached a conclusion that we can avoid solving this problem for custom elements for now.

1) Anne presented the fact that we will need "adopt" callback for custom elements to adjust baseURL when a node is adopted into a document. The same callback could be used (as a UA implementation detail) to swizzle prototypes for engines that do want to change the prototype and do nothing for engines that don't.

2) The adopt callback will accept two arguments: old document and new document.

3) A new registry API will need to exist to expose the "registry" concept in the current custom elements spec: http://w3c.github.io/webcomponents/spec/custom/#dfn-registry. This registry will be exposed as a member of a Document object and allow the adopt callback reason about what to do with the element definitions across documents. The registry will likely look like a map of definitions.

I will take an action item to work on the registry spec and refactor custom elements spec to depend on it.
Comment 70 Anne 2014-02-07 10:48:37 UTC
I recall 1) slightly differently. I will define in DOM that UAs must change prototypes. Whether that allows the old global to be actually GC'd is an implementation detail (as GC is in general).

I filed bug 24577 and bug 24578 on the custom element specification to add the callback 2) and expose the registry 3).
Comment 71 Adam Barth 2014-02-10 23:58:04 UTC
Long thread is long.

(In reply to Anne from comment #70)
> I recall 1) slightly differently. I will define in DOM that UAs must change
> prototypes.

I'm not sure everyone agrees that's the most desirable thing for DOM to require...  If the two of you don't recall the conversation in the same way, wouldn't it make more sense to clear up that miscommunication first?
Comment 72 Bobby Holley (:bholley) 2014-02-11 05:32:54 UTC
(In reply to Adam Barth from comment #71)
> Long thread is long.
> 
> (In reply to Anne from comment #70)
> > I recall 1) slightly differently. I will define in DOM that UAs must change
> > prototypes.
> 
> I'm not sure everyone agrees that's the most desirable thing for DOM to
> require...  If the two of you don't recall the conversation in the same way,
> wouldn't it make more sense to clear up that miscommunication first?

My recollection is that Dmitri's (1) was raised at one point, to which Boris asked "so are we just going to call this undefined and go home?" Anne wasn't enthusiastic about this option.

Dmitri explained that he had talked to abarth during the fire alarm, and that abarth had indicated that globals were baked into objects in V8, and that moving objects between globals wasn't really an option in Blink. Anne (?) then asked if there was a reason Blink couldn't just swizzle the prototype (to keep observables the same) and leave the underlying state (not taking advantage of the leak-plugging, but matching the observables of Trident and Gecko, which do). Boris pointed out that this must be possible, because __proto__ is script-mutable in Blink. I don't recall Dmitri objecting to this, though to be fair it wasn't really his topic of interest nor within his domain of authority.
Comment 73 Anne 2014-02-13 12:22:59 UTC
That matches mine.

I don't want undefined behavior here. That results in way too subtle bugs downstream.