Can the same constructed stylesheet be used in two different Documents?

[rakina]

Should adding the same constructed stylesheet in two different Documents be allowed? If it is, can it be used as a security attack vector?

[calebdwilliams]

For the purposes of this question does "documents" include document fragments (so shadow roots) or just a window.document?

[ewilligers]

This question is about different window.document instances, e.g. from different origins. The same constructed stylesheet can certainly be used in many shadow roots under the same document - that is the primary use case.

[calebdwilliams]

I would be weary of that possibility because I can see foresee instances where library authors might want to check if the object they get is an instance of CSSStyleSheet. If the constructors aren’t the same, that might cause an issue (unless I’m missing the point).

[TakayoshiKochi]

I think if once a constructed stylesheet is associated with any browsing context (i.e. specified for
actual rendering), then reusing that same object to another browsing context would throw an exception
(e.g. at the point that the object is added to .moreStyles or whatever it ends up with).

[TakayoshiKochi]

Let's conclude that a constructed stylesheet can only be associated with one document.
I'll close this once corresponding spec draft change is done.

[rakina]

#35 resolves this. It can't be used in multiple Documents.

[rakina]

Reopening after @annevk's comment on WICG/webcomponents#759 (comment). Maybe it's OK to make a stylesheet be usable in more than one Documents if we clearly define things that need to be defined?

According to #10, the URL of the stylesheet will stay the same, that is the document it is constructed on. For fetch group, we might want to use the one for the document it's constructed on? Not sure, I'll try to find out...

One of the other things that was pointed out was the security aspect - can we modify another Document's <style> element's stylesheet? If we can't, then maybe it make sense to not allow injecting a constructed stylesheet to another Document.

cc @tabatkins, who might be able to help solve these questions

[rakina]

Resolved with #61.

[rakina]

Current status is to ignore stylesheets from a different document tree. Reopening for further discussion from #69 (review) @emilio @bzbarsky @domenic @annevk @lilles

[emilio]

Is it very annoying implementation-wise to change the adoption algorithm in Blink? At least in Gecko we do need to iterate over the whole subtree including shadows, so I'd be surprised if that was hard to implement.

I'd also be a bit surprised if Blink didn't have to go through every adopted node and shadow tree as well.

[domenic]

I'll let @rakina answer on implementability, but from what I remember at TPAC most folks were loathe to add steps to the generic DOM tree adopting algorithm that support a CSSOM-specific feature. That felt like a layering violation. I guess upon reflection it might not be so bad; we'd just define the adopting steps for all shadow roots as doing this CSSOM stuff.

We additionally had the issue of how to surface errors. If the purpose is to prevent adoption, then we'd have to throw from the adopt algorithm. But that currently isn't done at all, which could have wider-ranging consequences. I think people had a few tricky examples, e.g. copying and pasting across documents.

[bzbarsky]

So my main concern with the proposed setup is that it requires the stylesheets in the adopted shadow root to hold on to the document they came from originally (via "constructor document") and thus keep it alive, even if it's now been unloaded, etc.

@emilio and I talked this over and we'd like to propose two things:

1. Setting adoptedStyleSheets should throw if the new list contains sheets that have the "wrong" constructor document.
2. Adopting a ShadowRoot clears out its adoptedStyleSheets.

That will guarantee that the sheets in adoptedStyleSheets always match the node document of the node involved, and prevent the accidental introduction of these cross-document memory leaks in web pages.

Thoughts?

[bzbarsky]

Can you explain why weak references wouldn't be possible

Nothing in any spec right now requires it being possible to weakly reference documents, afaict.

or why it would be bad for the document to be null when nobody else has a reference to it?

Hmm. I though we were still using the constructor document to detect constructed sheets, but looks like we have a separate "constructed flag" for that now. So I was just wrong there.

Hmm, I feel the same way about making changes to adopt-a-node plus the adoptedStyleSheets setter.

We'd just need to define adopting steps, not actually change adopt-a-node itself, right? I agree that it's not clear how this maps onto what implementations do.

The "ignore the sheets" proposal just adds an extra filter clause to the list of sheets that apply

Well, plus the weak ref machinery. I agree that given the "constructed flag" it should be possible to spec the weak ref stuff the way it should be implemented, as long as all implementations are on board with it.

[domenic]

Thanks, seems like we're iterating toward two potentially-workable options. Here's my attempt at a summary, hopefully not too biased:

• Ignore the sheets (current spec as of Clarify loading stuff and disallow multiple document usage for now #69)
  • (+) A bit simpler/fewer moving pieces
  • (-) Requires Gecko to do weak references and extra proto-swizzling to maintain their anti-leaking-documents-on-adoption protection, increasing Gecko's delta from the adopting spec
• Prohibit the sheets (proposed upthread in Can the same constructed stylesheet be used in two different Documents? #23 (comment))
  • (+) More obvious to developers they're doing something "weird", at least when using the setter
  • (-) Requires introducing the third set of adopting steps in the platform, in a way that feels vaguely like a layering-violation (ShadowRoot depending on CSS)

Note that I think both options would need an opt-in if we ever discover a use case and want to allow cross-document usage in the future.

With these in hand, and remembering my priority of constituencies, I'm leaning a bit toward "prohibit the sheets". But I'd love to step back and give others a chance to comment or expand the pros/cons list.

[annevk]

So is the big problem here the fetch group? Hold by the document and used by the style sheet. And a fetch group gets all its fetches canceled when a document's browsing context is navigated or goes away. And using the fetch group from the new document would mess up already ongoing fetches.

Does the attempt to adopt have any impact on ongoing fetches in either solution? I.e., are they canceled? I guess since they are ignored/prohibited we'd just leave them alone.

I guess the involved solution here is for the style sheet to have its own fetch group which is reference counted from the various fetch groups of the documents it's adopted in. We have nested fetch groups (not really specified though), but the reference counting aspect would be novel.

[domenic]

Hmm, I'm not sure whether the fetch group is relevant to ignore vs. prohibit. The current spec says

Note: We will use the fetch group of sheet’s constructor document's relevant settings object for @import rules and other (fonts, etc) loads.

Note: The rules regarding loading mentioned above are currently not specified rigorously anywhere.

which seems "good enough"...

[annevk]

I meant that the reason folks want this strongly tied to a single document is the fetch group architecture, which albeit poorly defined, is a thing browsers have.

[rakina]

I meant that the reason folks want this strongly tied to a single document is the fetch group architecture, which albeit poorly defined, is a thing browsers have.

Yes, it was a big reason (see #15) for not allowing multiple Document usages for now (along with complexity in implementation?)

[rakina]

After thinking about it, I think it is better to prohibit the sheets and clear the adopted sheets on adoption to a different document like @bzbarsky's comment on #23 (comment). Blink's implementation had been updated, and I'll update the spec soon.

[heycam]

@rakina can this issue be closed? I think your spec changes to follow the #23 (comment) proposal were made.

[rakina]

Oops yeah forgot to actually close the issue. Thanks!