This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 21976 - [imports]: Preventing DOM hierarchy cycles
Summary: [imports]: Preventing DOM hierarchy cycles
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: DOM (show other bugs)
Version: unspecified
Hardware: PC All
: P2 normal
Target Milestone: ---
Assignee: Anne
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 20683
  Show dependency treegraph
 
Reported: 2013-05-08 20:35 UTC by Rafael Weinstein
Modified: 2013-10-04 07:35 UTC (History)
6 users (show)

See Also:


Attachments

Description Rafael Weinstein 2013-05-08 20:35:32 UTC
Seems like we'd like to disallow a hierchy cycle via link imports, the same way we do for template.content and element->shadowroot.

append/insert child check the http://dom.spec.whatwg.org/#concept-tree-host-including-inclusive-ancestor.

however, if a document can be linked by multiple link.import.content, then it essentially has multiple hosts and thus the host-inclusive-ancestor must walk a tree-shaped (rather than linear) ancestry chain.
Comment 1 Anne 2013-05-08 22:42:01 UTC
So bug 21255 suggests HTML imports will use DocumentFragment. I don't understand how they could reuse the same object for that.
Comment 2 Anne 2013-08-16 11:08:42 UTC
Can someone please explain this? I'd like to have this covered.
Comment 3 Dimitri Glazkov 2013-08-16 16:03:38 UTC
(In reply to comment #2)
> Can someone please explain this? I'd like to have this covered.

Rafael is probably the right guy to write a much clearer explanation, but I'll try first :)

Suppose you have the following import structure:

index.html:
<link rel=import href=a.html id=i-a>
<link rel=import href=b.html id=i-b>

a.html:
<link rel=import href=b.html id=a-b>

b.html:
<link rel=import href=c.html id=b-c>

c.html:
<link rel=import href=a.html id=c-a>

If we suppose that only one import is loaded per URL (that's what the spec says at the moment), b.html's parent imports are both index.html and a.html, which creates the tree-shaped ancestry chain that Rafael mentions.

A naive fix would be to always load a different import for a URL, but now we can't easily resolve cycles in the import tree. For example, when loading c, it's unclear what to do with #c-a.
Comment 4 Anne 2013-08-19 11:10:23 UTC
You'll have to explain to me how this is a better way to deal with the situation that also arises with <iframe> and CSS @import. Always creating a fresh instance and have stack limits seems more consistent.
Comment 5 Rafael Weinstein 2013-08-19 17:53:04 UTC
I'd explain this by analogy to the template element.

Consider

<div id=a>
  <template>
    <div id=b>

If we parse this, we need a way to prevent a cycle (for example by appending divA as a child to divB).

The way we accomplish this is that the templates content document fragment has a (non-exposed) |host| pointer which points at its host (the template element) --- and we have append/insert check that the host-including-inclusive-ancestor, which traverse this host link to check for a cycle.

Now consider the what would happen if a single document fragment would be shared among template elements: What does its |host| point to? It has multiple "hosts"? Does append/insert have to traverse all of them?

Because HTML imports allows multiple link.document pointers to point to the same document, we have this later case. Now consider something like

<div id=a>
  <link rel="import" href="a.html">
  <link rel="import" href="a.html">

----a.html----
<div id=b>

How to prevent a cycle by attempting to append divA as a child of divB (which is a child of a document which is pointed to by both link.documents.
Comment 6 Anne 2013-08-20 15:04:36 UTC
Right. My question is why do we allow those DocumentFragment pieces to be identical structures? That's not how CSS @import works, or <iframe>, ...


Now, assuming there's a good reason. We'd need to allow a list of hosts for DocumentFragment. And then expand the definition of host-including inclusive ancestor to thus:

"An object /A/ is a host-including inclusive ancestor of an object /B/ if either /A/ is an inclusive ancestor of /B/, or if /B/'s root has one or more associated hosts and /A/ is a host-including inclusive ancestor of one of /B/'s root's hosts."

Right?
Comment 7 Rafael Weinstein 2013-08-22 19:17:17 UTC
I think this is really for Dimitri is address, but I think the reason is that, during development, you may have lots of components, each in it's own file. Many components may "depend" on a given other component, and they all want to "import" it, but you don't want to import it once for each component that wants it. You want to import it once and let all them share it.

I don't know what the right solution is. Having multiple hosts seems scary.
Comment 8 Anne 2013-08-23 10:01:25 UTC
The other thing I've been pondering is that if HTML imports is going to be a thing, it better be defined in HTML. Having something so fundamental defined separately is does not seem good.

Is there consensus among implementers that HTML imports is going to happen?
Comment 9 Dimitri Glazkov 2013-08-23 19:38:12 UTC
(In reply to comment #8)
> The other thing I've been pondering is that if HTML imports is going to be a
> thing, it better be defined in HTML. Having something so fundamental defined
> separately is does not seem good.

Totally agree. This spec should follow the same path as HTML Templates.
 
> Is there consensus among implementers that HTML imports is going to happen?

Blink is implementing it, I am pretty sure Gecko will too.
Comment 10 Dimitri Glazkov 2013-09-18 20:01:32 UTC
Morrita-san pointed out that imports have no corresponding |host| accessor (and have no intention of adding it), which neatly resolves cyclic problems for HTML Imports.
Comment 11 Rafael Weinstein 2013-09-18 20:13:46 UTC
That prevents the upward cycle, but the downward cycle is still possible (e.g. example in comment 5).

What am I missing?
Comment 12 Morrita Hajime 2013-09-18 20:40:52 UTC
(In reply to Rafael Weinstein from comment #11)
> That prevents the upward cycle, but the downward cycle is still possible
> (e.g. example in comment 5).
> 
> What am I missing?

You are right.
Naively thinking, we can just throw an exception in that case.
Comment 13 Rafael Weinstein 2013-09-19 13:42:14 UTC
Well, that's the question. DOM hierarchy errors throw when they are detected.

The questions are:

1) Do we want to prevent cycles in this case (and if not, why is this different from other cases)?

2) How do we detect the cycle?
Comment 14 Morrita Hajime 2013-10-01 09:44:34 UTC
On second thought, I think we can just let create the cycle.
As Raf pointed, detecting cycle isn't trivial and will be expensive.

Instead, we can make the <link>-to-import reference weak.
We can do this because the import can be retained by someone else,
probably by the master document.
The <link> doesn't need to be an owner of the import.

Conceptually, <link rel=import> could be taken as a directive to load the import.
It doesn't need to imply any ownership. 

Implementation wise, we could maintain import dependency tree in a separate
data structure. (Blink is doing this.) So loaded imports are alive as long as
the dependency structure is retained by, say, the master document.

So, what should happen when the master document is gone while the <link> is
alive somewhere else? This can happen if the <link> is moved to another frame
and original frame is removed.

In this case, I think it is OK to disconnect <link> from the imported document,
and allow |link.import| to return null.
This is useless edge case where we just have to prevent terrible things
(leak, crash) from happening.

Does this make sense?
Comment 15 Anne 2013-10-01 17:25:47 UTC
That sounds rather bad. Does that make garbage collection observable? Not even for edge cases is that okay, we should definitely avoid it for new designs.

Who from Gecko is interested in this? Would be good to have their implementation input.
Comment 16 Olli Pettay 2013-10-01 17:35:10 UTC
(In reply to Morrita Hajime from comment #14)

> In this case, I think it is OK to disconnect <link> from the imported
> document,
> and allow |link.import| to return null.
> This is useless edge case where we just have to prevent terrible things
> (leak, crash) from happening.
> 
> Does this make sense?
return null doesn't sounds ok to me.


Gecko would just use cycle collector for this kind of stuff, and objects would
stay alive as long as they can be accessed from JS.
Comment 17 Morrita Hajime 2013-10-03 09:25:41 UTC
OK, Blink doesn't have cycle collector and needs some tweak.
And here is another option: We could let link.import return null more eagerly.

Currently the spec says that link.import returns null when it is out of document.
That means it comes back to non-null once it is (re)inserted into a document.

This could be more strict: Once the <link> is removed from a document,
its link.import should return null thereafter, regardless the element is 
re-inserted into any document.
In other word, the <link> dies if it is removed from the document,
and it does never come back.

This makes the lifetime a bit simple and keep GC side-effect from visible.
Comment 18 Morrita Hajime 2013-10-04 07:35:10 UTC
Trying to fix it by eargly nullifying link.import
https://dvcs.w3.org/hg/webcomponents/rev/c6b1d1350ac8

Please let me know what you think!