At least https://searchfox.org/mozilla-central/rev/d69aac56879ce3adf78e9fb23732d2d3b9fa259d/layout/style/Loader.cpp#2181 looks pretty wrong in presence of Shadow DOM. But most of the callers of GetAssociatedDocument really want something like the composed doc of the owner node, instead of the owning document.

Boris, can you think of any reason we couldn't just remove mDocument / mDocumentAssociationMode and go through the owner node as appropriate? The IsOwnedByDocument() bit in Rule::IsKnownLive is the only one I'm not quite sure it's fine to replace it with that.

Owner node / Owner node + parent sheet chain, I mean.

> Boris, can you think of any reason we couldn't just remove mDocument / mDocumentAssociationMode and > go through the owner node as appropriate? There are sheets that are associated with a document (and often owned by it) but not a particular node. For example: * Sheets loaded via nsDOMWindowUtils::LoadSheet. * Some editor sheets. * Sheets added via nsIDocument::AddStyleSheet (the XUL overlay thing will likely go away, leaving the editor bit and static clone bit). * Whatever mess nsIDocument::UpdateStyleSheets is doing. * Sheets loaded via HTTP Link headers. Going back to that GetAssociatedDocument() caller in Loader::LoadChildSheet, that's just an optimization, no? if there's no associated document there is _definitely_ no owner node and there is no point walking up the parent sheet chain. This probably mattered more when this walk looked more like this: nsCOMPtr<nsIDOMStyleSheet> nextParentSheet(do_QueryInterface(aParentSheet)); NS_ENSURE_TRUE(nextParentSheet, NS_ERROR_FAILURE); //Not a stylesheet!? nsCOMPtr<nsIDOMStyleSheet> topSheet; //traverse our way to the top-most sheet do { topSheet.swap(nextParentSheet); topSheet->GetParentStyleSheet(getter_AddRefs(nextParentSheet)); } while (nextParentSheet); topSheet->GetOwnerNode(getter_AddRefs(owningNode)); as in https://searchfox.org/mozilla-central/rev/fc48c463fc71d38a767b630bd0ab17583bec73d5/layout/style/nsCSSLoader.cpp#1766-1776 Anyway, this caller probably cares about composed doc, except insofar as we shouldn't really land here if there is an owner node and it's not document-connected, right? I just glanced through the callers of GetAssociatedDocument: 1) nsDOMWindowUtils::AddSheet: Cares about actual document association, so a single sheet doesn't get associated with multiple documents. In the case of having an owner node, wants the owner doc. 2) SizeOfOwnedSheetArrayExcludingThis: Cares about actual document association to determine which sheets we actually own. Wants owner doc. 3) ErrorReporter::ShouldReportErrors: Is caring about owner docs, clearly. 4) Loader::LoadChildSheet: see above. 5) MediaList::DoMediaChange: Cares about whatever document the sheet would be styling. Probably really wants composed doc, but can this get called for non-document-connected owner nodes? 6) Rule::IsKnownLive: Cares about what document owns the sheet. Doesn't care how or why it owns it, exactly, and should ideally work for cases like HTTP Link headers. 7) Rule::GetDocument: Would need to audit callers. Haven't done that. 8) ServoCSSRuleList::InsertRule: Needs to be able to reach the loader for "the document". You can insertRule() an @import rule into an HTTP Link header sheet, I would think, and I would expect that to work... We should make sure this is tested. 9) ServoStyleRuleDeclaration::SetCSSDeclaration: As #5: what matters is what document is being styled, if any. 10) ServoStyleRule::SetSelectorText: As #5.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3) > Going back to that GetAssociatedDocument() caller in Loader::LoadChildSheet, > that's just an optimization, no? if there's no associated document there is > _definitely_ no owner node and there is no point walking up the parent sheet > chain. That is not true, I guess that's the key. Sheets in Shadow DOM don't get associated to a document, yet they get an owner node, affect styling, etc... I guess another way to look at this bug is saying: Should stylesheets in shadow DOM get associated to a document? That makes the semantics of GetAssociatedDocument somewhat confusing. If so, what should happen when the shadow root gets moved away from the associated document? Should we de-associate its stylesheets? I guess that already happens, but it leaves ShadowRoot with a weird "sheets are not associated to anything", yet I own them. I guess I can change AssociatedDocument to AssociatedDocumentOrShadowRoot instead, that may be cleaner. Then GetAssociatedDocument would be something like: if (!mDocumentOrShadowRoot) return nullptr; return mDocumentOrShadowRoot->AsNode().GetComposedDoc(); Or something of the sort. That way if the shadow root is not connected to the document the "associated document" would be null.

> Sheets in Shadow DOM don't get associated to a document That seems wrong to me, at first glance. > Should stylesheets in shadow DOM get associated to a document? I'd think so. I mean, mutations to those sheets should notify via the document observer mechanism, right? > If so, what should happen when the shadow root gets moved away from the associated document? Moved away in the sense that the shadow host is not document-connected? In normal DOM subtrees, making them not document-connected drops the sheets. It sounds like that's not true for shadow DOM, so we are getting invariant violations? :(

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5) > In normal DOM subtrees, making them not document-connected drops the sheets. > It sounds like that's not true for shadow DOM, so we are getting invariant > violations? :( Right, the thing that owns the sheet is the ShadowRoot itself, thus my proposal of changing the member to be a DocumentOrShadowRoot instead. ShadowRoots are never document-connected, so there's no way of conflict here, and when you want a document you can use the ShadowRoot's GetComposedDoc. WDYT?

So ShadowRoot's GetComposedDoc returns exactly what the shadow host's GetComposedDoc() returns? And in particular if it returns non-null we know the document is actually holding on to us? That seems like it would work fine at first glance. We'd need to adjust IsOwnedByDocument() on sheets in shadow trees when the shadowroot's mIsComposedDocParticipant changes, to make IsKnownLive work correctly. It looks like ShadowRoot has its list of sheets, so this would not be too hard, right?

Comment on attachment 8975077 [details] Bug 1459529: Make sheets be associated to a shadow root too potentially. https://reviewboard.mozilla.org/r/243420/#review249392 I wonder whether tests for dynamic mutation of sheets in shadow DOM would catch some of those places where we didn't notify.... Worth checking. r=me modulo the naming issue with GetOwnerDoc() that I mention below. ::: dom/base/DocumentOrShadowRoot.h:131 (Diff revision 1) > void ElementsFromPointHelper(float aX, float aY, uint32_t aFlags, > nsTArray<RefPtr<mozilla::dom::Element>>& aElements); > > protected: > + // Returns the reference to the sheet, if found in mStyleSheets. > + RefPtr<StyleSheet> RemoveSheet(StyleSheet& aSheet); The normal pattern is to return already_AddRefed. If there's a reason not to do that here, it needs to at least be documented. ::: dom/base/DocumentOrShadowRoot.cpp:57 (Diff revision 1) > + > +RefPtr<StyleSheet> > +DocumentOrShadowRoot::RemoveSheet(StyleSheet& aSheet) > +{ > + auto index = mStyleSheets.IndexOf(&aSheet); > + if (index == nsTArray<RefPtr<StyleSheet>>::NoIndex) { You could probably write that more clearly as mStyleSheets.NoIndex. ::: dom/base/ShadowRoot.h:40 (Diff revision 1) > class ShadowRoot final : public DocumentFragment, > public DocumentOrShadowRoot, > public nsStubMutationObserver > { > public: > - static ShadowRoot* FromNode(nsINode* aNode) > + NS_IMPL_FROMNODE_HELPER(ShadowRoot, IsShadowRoot()); Unrelated-ish cleanup, I assume? It's fine; just want to make sure I'm not missing something. If it is, maybe put it in a separate changeset. ::: layout/style/Loader.cpp:2163 (Diff revision 1) > > LOG_URI(" Child uri: '%s'", aURL); > > nsCOMPtr<nsINode> owningNode; > > // Check for an associated document: if none, don't bother walking up the Fix this comment to match code? ::: layout/style/StyleSheet.h:260 (Diff revision 1) > }; > - nsIDocument* GetAssociatedDocument() const { return mDocument; } > - bool IsOwnedByDocument() const { > - return mDocumentAssociationMode == OwnedByDocument; > - } > - // aDocument must not be null. > + dom::DocumentOrShadowRoot* GetAssociatedDocumentOrShadowRoot() const > + { > + return mDocumentOrShadowRoot; > + } > + bool IsKeptAliveByDocument() const; Please document what this function actually returns. And please document the relationship between this function and GetComposedDoc() that the rule IsLive code is relying on: if IsKeptAliveByDocument() returns true, then GetComposeDoc() will definitely return true. ::: layout/style/StyleSheet.h:265 (Diff revision 1) > - // aDocument must not be null. > - void SetAssociatedDocument(nsIDocument* aDocument, DocumentAssociationMode); > - void ClearAssociatedDocument(); > + bool IsKeptAliveByDocument() const; > + > + // Returns the document whose styles this sheet is affecting. > + nsIDocument* GetComposedDoc() const; > + > + // Returns the document we belong to, if any. "belong", and the name of this function implies ownership, but the document may not own this sheet in the OwnedByDocumentOrShadowRoot sense.... I'm not sure how to best name things here but the current naming is not making me very happy.

Comment on attachment 8975077 [details] Bug 1459529: Make sheets be associated to a shadow root too potentially. https://reviewboard.mozilla.org/r/243420/#review249392 > The normal pattern is to return already_AddRefed. If there's a reason not to do that here, it needs to at least be documented. Only one of the two callers cared about it, so I didn't want to force them to grab a reference just so it went out of scope. But I think I ended up using the return value in both so... > You could probably write that more clearly as mStyleSheets.NoIndex. Agreed, though then there's stuff like bug 1443080 that changed most uses of this pattern to what I wrote here. I prefer `mStyleSheets.NoIndex` though, so I guess I'll use it anyway. > Unrelated-ish cleanup, I assume? It's fine; just want to make sure I'm not missing something. If it is, maybe put it in a separate changeset. Well, I started using the `FromNode(nsINode&)` version, so not quite unrelated. > "belong", and the name of this function implies ownership, but the document may not own this sheet in the OwnedByDocumentOrShadowRoot sense.... > > I'm not sure how to best name things here but the current naming is not making me very happy. Agreed... Will think a bit about it, "GetRelatedDocument"? Also not great :(

> I wonder whether tests for dynamic mutation of sheets in shadow DOM would catch some of those places where we didn't notify.... Worth checking. Also, this is not a problem because all those mutations end up in ShadowRoot::ApplicableRulesChanged, which notifies itself. Thus the following commits to unify this setup.

Comment on attachment 8975077 [details] Bug 1459529: Make sheets be associated to a shadow root too potentially. https://reviewboard.mozilla.org/r/243420/#review249392 > Only one of the two callers cared about it, so I didn't want to force them to grab a reference just so it went out of scope. But I think I ended up using the return value in both so... > But I think I ended up using the return value in both Right, hence my suggestion. > Agreed... Will think a bit about it, "GetRelatedDocument"? Also not great :( OK, I thought about this some more. The node ownerDocument precedent, where the document doesn't actually own the node, is pretty annoying.... Anyway, we could just keep GetAssociatedDocument there (with a rename on top of the current patches). The way to think of this, ideally, is that this getter is returning a thing that fundamentally should exist in the CSSOM spec (but doesn't; there are some issue numbers in https://drafts.csswg.org/cssom/#requirements-on-user-agents-implementing-the-http-link-header about that). Does that help with the naming in any way? I'm not sure it does. :(

Comment on attachment 8975078 [details] Bug 1459529: Remove ServoStyleSet::{Begin,End}Update. https://reviewboard.mozilla.org/r/243422/#review249830

Comment on attachment 8975079 [details] Bug 1459529: Remove PresShell::BeginUpdate. https://reviewboard.mozilla.org/r/243424/#review249836

With PresShell::BeginUpdate removed, there might be a better name we could use for PresShell::EndUpdate.

Comment on attachment 8975080 [details] Bug 1459529: Remove some useless nsIDocumentObserver notifications. https://reviewboard.mozilla.org/r/243426/#review249838 Nice to see these go.

Comment on attachment 8975081 [details] Bug 1459529: Remove UPDATE_STYLE. https://reviewboard.mozilla.org/r/243428/#review249846 ::: commit-message-8ac5a:7 (Diff revision 2) > +There's a big hidden gotcha here. nsIDocument::BeginUpdate does put a script > +blocker on the stack for these updates. However it's not needed, since no script > +can run during these notifications (only the stylesheet events we post for > +devtools, but those use AsyncEventDispatcher and PostDOMEvents, so they don't > +try to run immediately). > + > +nsIDocument::BeginUpdate also does XBL binding attached queue stuff, but we > +can't change bindings during these notifications anyway, so it also doesn't > +matter. I think you're right about this stuff. But I would greatly appreciate bz taking a second look to confirm. (It would be nice to have some greater guarantees about not running script in certain situations, maybe by turning the BeginUpdate/EndUpdate/mozAutoDocUpdate calls into things that perform assertions, but we're not great about that in general, so...) ::: dom/events/IMEContentObserver.cpp (Diff revision 2) > - if (!(aUpdateType & UPDATE_CONTENT_MODEL)) { > - return; > - } Do we ever pass in 0 as the update type to nsIDocument::BeginUpdate? If we do, then we still need this check. If we don't (and I think we don't), can you remove it in a followup?

Boris, could you skim over the commit message in the comment above to sanity-check? Thanks!

> since no script can run during these notifications This is _really_ hard to prove in general... I certainly haven't managed to prove it to my satisfaction here. That said, it sounds like no real work is done during the notifications other than posting async stuff of various sorts? If so, we should probably be ok. My main worry is that people will add something later in the mass of code that runs there that will be unsafe. Maybe what we should do is have an annotation like MOZ_CAN_RUN_SCRIPT but for functions that are known to _not_ run script. Such functions would only be allowed to call other functions that are thus annotated... Worth a followup?

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/699f1d745e76 Make sheets be associated to a shadow root too potentially. r=bz https://hg.mozilla.org/integration/autoland/rev/f04194f1a3d0 Remove ServoStyleSet::{Begin,End}Update. r=heycam https://hg.mozilla.org/integration/autoland/rev/0c1c3fb200c0 Remove PresShell::BeginUpdate. r=heycam https://hg.mozilla.org/integration/autoland/rev/6695fc0e5289 Remove some useless nsIDocumentObserver notifications. r=heycam https://hg.mozilla.org/integration/autoland/rev/a40accc5c91c Remove UPDATE_STYLE. r=heycam

https://hg.mozilla.org/mozilla-central/rev/699f1d745e76 https://hg.mozilla.org/mozilla-central/rev/f04194f1a3d0 https://hg.mozilla.org/mozilla-central/rev/0c1c3fb200c0 https://hg.mozilla.org/mozilla-central/rev/6695fc0e5289 https://hg.mozilla.org/mozilla-central/rev/a40accc5c91c