1170888 - EnsureSafeToHandOutCSSRules is not enough to ensure inDOMUtils::GetRuleNodeForContent gets an up-to-date rule node

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

My patch in bug 1169512 reveals a problem with inDOMUtils::GetRuleNodeForContent resulting in an intermittent failure of toolkit/devtools/server/tests/mochitest/test_styles-computed.html (e.g. in https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c45d3c5a917).

The problem is basically this:

1. The CSS Loader has a cached copy of a style sheet used by the test.
2. The test loads the style sheet through a <link>, which results in the cached copy being cloned and added to the style set.  The cloned sheet still shares an inner with the cached sheet.
3. We construct frames in the document.  One of the elements in the document matches a rule from the style sheet, and the style context we give it has a rule node that points to that rule.  Since we haven't called EnsureUniqueInner on the cloned sheet, this rule's mSheet is the original, cached sheet (which is not in the style set).
4. The test calls inDOMUtils::GetCSSStyleRules -> inDOMUtils::GetRuleNodeForContent -> nsComputedDOMStyle::GetStyleContextForElement, which, if the element already has a frame, just returns the frame's style context.  GetRuleNodeForContent calls nsPresContext::EnsureSafeToHandOutCSSRules before the GetStyleContextForElement call, which does call EnsureUniqueInner on the sheet, but this is not enough to restyle the element's frame.  Thus, GetCSSStyleRules ends up returning rules from the original, cached sheet.
5. The test gets the rule's parentStyleSheet and looks at its ownerNode and finds it is null.  But really we expect ownerNode to be the <link> element that loaded the style sheet.

The simplest way to solve this might be to have GetRuleNodeForContent force GetStyleContextForElement to create a new style context.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b04e9cbfd2c

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm a little worried about performance, although maybe this is a codepath where the performance isn't so critical?  It still seems a little fishy to even add this option.

Would it work to add a flush somewhere that would flush the buffered-up changes?

Comment 3

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #2)
> I'm a little worried about performance, although maybe this is a codepath
> where the performance isn't so critical?  It still seems a little fishy to
> even add this option.

Yeah, ideally we wouldn't need to, and any cached style context found would be safe to return.  The devtools code does a lot of caching of things, so I don't think it's going to be a problem performance-wise.

> Would it work to add a flush somewhere that would flush the buffered-up
> changes?

We already do flush style in nsComputedDOMStyle::GetStyleContextForElement, but we don't have a restyle pending for the element.  Maybe nsPresContext::EnsureSafeToHandOutCSSRules could post some restyles but we'd have to do this for all elements whose frames' style contexts point to rule nodes whose mRule comes from the style sheet's old inner.  That's going to be more wasteful than just forcing creation of a new style context here.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8614476 [details] [diff] [review]
patch

I'm actually a little bit surprised that we don't restyle everything
after calling EnsureUniqueInner -- though I suppose I can't think of a
reason that we need to.  It seems a little scary, though; I feel like
there ought to be a big comment pointing it out somewhere, but I can't
think of where.  Maybe in EnsureUniqueInner?  (EnsureUniqueInner calls
ClearRuleCascades so that all future rule matching uses the new inner,
but it doesn't restyle existing style contexts.)

Maybe also add a comment above GetStyleContextForElement explaining
that you might change eStyleContextResolution if you need the style
context to have up-to-date rules following an EnsureUniqueInner because
you're going to hand those rules out via an API.

Maybe an alternative is to have a notion that there's a pending
unprocessed EnsureUniqueInner?  You could then rename
eResolveStyleContext_Always to eProcessPendingUniqueInnerRestyles.

But I guess this is fine as-is, and we don't need the extra complexity
unless this is shown to be a performance problem.

So r=dbaron with that.

Comment 5

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⏰UTC-4 (mostly busy, back June 29) from comment #4)
> Comment on attachment 8614476 [details] [diff] [review]
> patch
> 
> I'm actually a little bit surprised that we don't restyle everything
> after calling EnsureUniqueInner -- though I suppose I can't think of a
> reason that we need to.  It seems a little scary, though; I feel like
> there ought to be a big comment pointing it out somewhere, but I can't
> think of where.  Maybe in EnsureUniqueInner?  (EnsureUniqueInner calls
> ClearRuleCascades so that all future rule matching uses the new inner,
> but it doesn't restyle existing style contexts.)

Yeah I think generally we don't need to restyle, as the old rule nodes will have exactly the same style information.  It's just the mRule that's different.  So restyling everything after EnsureUniqueInner seems like it would be wasted work unless you need an accurate mRule.

> Maybe also add a comment above GetStyleContextForElement explaining
> that you might change eStyleContextResolution if you need the style
> context to have up-to-date rules following an EnsureUniqueInner because
> you're going to hand those rules out via an API.

OK (assuming you mean "might pass").

> Maybe an alternative is to have a notion that there's a pending
> unprocessed EnsureUniqueInner?  You could then rename
> eResolveStyleContext_Always to eProcessPendingUniqueInnerRestyles.

I think it's probably clearer to describe that it's forcing the resolution of a style context, rather than "processing" the result of calling EnsureUniqueInner, which would then need some additional explanation.  But I'll add a comment before the GetStyleContextForElement call to say why we do need to pass eResolveStyleContext_Always.

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #5)
> > I'm actually a little bit surprised that we don't restyle everything
> > after calling EnsureUniqueInner -- though I suppose I can't think of a
> > reason that we need to.  It seems a little scary, though; I feel like
> > there ought to be a big comment pointing it out somewhere, but I can't
> > think of where.  Maybe in EnsureUniqueInner?  (EnsureUniqueInner calls
> > ClearRuleCascades so that all future rule matching uses the new inner,
> > but it doesn't restyle existing style contexts.)

I think I'll put this comment on nsPresContext::EnsureSafeToHandOutRules, since it already talks about the restyling that it does (which is insufficient here).

Comment 7

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⏰UTC-4 (mostly busy, back June 29) from comment #4)
> I'm actually a little bit surprised that we don't restyle everything
> after calling EnsureUniqueInner -- though I suppose I can't think of a
> reason that we need to.

Hmm, so actually why is the restyling in nsPresContext::EnsureSafeToHandOutRules insufficient here?  I've deleted the notes I made while debugging this, so I don't remember, but since we're restyling everything with eRestyle_Subtree, shouldn't we re-run selector matching under nsStyleSet::ResolveStyleFor to get a brand new rule node?

Comment 8

[Cameron McCormack (:heycam)]

I think I've worked it out.  The nsPresContext::EnsureSafeToHandOutRules call at this point discovers that all sheets in the style set already have unique inners, and that's because we happened to call EnsureUniqueInner earlier on, from within CSSRuleListImpl::IndexedGetter.  But that was an explicit EnsureUniqueInner call, not nsPresContext::EnsureSafeToHandOutRules, which means that we didn't actually restyle anything in response.

Comment 9

[Cameron McCormack (:heycam)]

I suppose what I'll do is record on CSSStyleSheet the fact that EnsureUniqueInner did clone the old inner, and that we haven't yet done a full restyle in response.  Then we can make nsStyleSet::EnsureUniqueInnerOnCSSSheets return eUniqueInner_ClonedInner if the "haven't restyled" flag hasn't been set yet.

Comment 10

[Cameron McCormack (:heycam)]

Though that's not exactly right, as the sheet might be used in multiple style sets...

Comment 11

[Cameron McCormack (:heycam)]

Attachment: patch v2

Let me know if you have any better ideas that don't involve storing the list of style sets on a CSSStyleSheet.  I don't think we can just use the mOwnerDocument of the CSSStyleSheet.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e5847281090

Comment 12

[Cameron McCormack (:heycam)]

Attachment: patch v2.1

Without crashes this time.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=314950255826

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8623626 [details] [diff] [review]
patch v2.1

Boris, would you have time to look at this?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8623626 [details] [diff] [review]
patch v2.1

>+  bool                  mShouldRestyleForClonedInner;

This seems to be write-only.  Please remove it.

>+    for (uint32_t i = 0, n = mSheets[type].Length(); i < n; i++) {

Any reason that's not a range loop like the other ones?

r=me with the dead bits removed; either way on the for loop.

Comment 15

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Comment on attachment 8623626 [details] [diff] [review]
> patch v2.1
> 
> >+  bool                  mShouldRestyleForClonedInner;
> 
> This seems to be write-only.  Please remove it.

Erk, remnant from a previous patch iteration.

> >+    for (uint32_t i = 0, n = mSheets[type].Length(); i < n; i++) {
> 
> Any reason that's not a range loop like the other ones?

nsCOMArray doesn't have begin/end methods on it, unfortunately. :(

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5dac565379

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c5dac565379