1127198 - ClearCachedInheritedStyleDataOnDescendants is sometimes called on style contexts which need to keep the old cached structs

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

This bug is for continuing the investigation into the issue identified in bug 1092363.

Comment 1

[Cameron McCormack (:heycam)]

The description of the problem in bug 1092363 comment 17 is partly correct, and my proposed solution for (b) is wrong.

Here is a restatement of the problem, hopefully correct this time.

Block(body)(1)@11c370010 [content=1003ed380] [sc=11c370a60]<
  Inline(span)(0)@11c370830 next=11c3b3250 IBSplitSibling=11c3b3250 [content=11c3697d0] [sc=11c370788]<
    Text(0)"a"@11c3709a0 [content=11c369860] [sc=11c36fd58:-moz-non-element] [run=11a357400][0,1,T]
  >
  Block(span)(0)@11c3b3250 next=11c3b3358 IBSplitSibling=11c3b3358 IBSplitPrevSibling=11c370830
                           [content=11c3697d0] [sc=11c370f40:-moz-anonymous-block,parent=11c370788]<
    Block(div)(1)@11c370c70 [content=1003edba0] [sc=11c3708f8,parent=11c370788]<
      Text(0)"b"@11c370db0 [state=00000040b0600000] [content=11c3698f0] [sc=11c370d08:-moz-non-element]
                           [run=11c36c080][0,1,T]
    >
  >
  Inline(span)(0)@11c3b3358 IBSplitPrevSibling=11c3b3250 [content=11c3697d0] [sc=11c370788]<>
>

and the style context tree starting from the <span> looks like this:

11c370788(5) Text=11c370a10(dependent) parent=11c370a60          <span>
  11c370f40(1) Text=11c370a10(dependent) :-moz-anonymous-block   <span>'s IB split block wrapper
  11c3708f8(2) Text=11c370a10(dependent)                         <div>
    11c370d08(1) Text=11c370a10(dependent) :-moz-non-element     "b"
  11c36fd58(1) Text=11c370a10(dependent) :-moz-non-element       "a"

1. We start with two restyles pending, one for the <span> and one for the <div>.
2. We start processing the first pending restyle by calling ComputeStyleChangeFor with the <span>'s primary frame.
3. ComputeStyleChangeFor enters its ib-split and different-style-continuation loops.
4. We call ElementRestyler::Restyle for the <span>'s primary frame.
5. ElementRestyler::Restyle calls RestyleSelf, which discovers no style changes.  This means we swap all of its structs to the new style context we just created for it.
6. ElementRestyler::Restyle calls RestyleChildren, which finds the "a" text frame, eventually calls RestyleSelf on it, and discovers no style changes.  This returns eRestyleResult_Stop and so we leave its old style context in place.
7. ElementRestyler::Restyle now checks the <span>'s primary frame's old style context's reference count.  It has more than 1, which means that after this Restyle call something else will still have a reference to the old style context.  The old style context may (and indeed does) have cached inherited style structs which were swapped from the old style context to the new style context.  That's bad, because if the new style context ever got destroyed, then these cached structs in the old style context's descendants would be dangling pointers.
8. We incorrectly assume that the > 1 ref count of the old style context means that something other than a frame is holding on to it.  However, both the <span>'s IB split block wrapper frame's style context AND the <div>'s frame's style context have a reference to it, as their parent.  Nevertheless, we call ClearCachedInheritedStyleDataOnDescendants on the <span>'s primary frame's old style context, which ends up clearing structs on the IB split block wrapper, the <div> frame, and the "b" text frame. 
9. Now we return up to ComputeStyleChangeFor and go through the IB loop to find the <span>'s IB block wrapper and call ElementRestyler::Restyle on it.
10. Eventually the restyle process gets down to restyling the "b" text frame, in CalcStyleDifference finds that the cached Text struct is null, so doesn't bother comparing it.  It we had not overwritten that struct with null, we would have compared it and discovered the different text-transform value.

We shouldn't have called ClearCachedInheritedStyleDataOnDescendants on the <span>'s primary frame's old style context, because we still had some frames to restyle which had style contexts that inherit from that old style context.

So I think we can avoid this by calling ClearCachedInheritedStyleDataOnDescendants a bit later -- specifically, at the end of the IB split loop up in ComputeStyleChangeFor.  We can keep an array of style structs we need to call this method on and do it after we know we've restyled all of the IB split parts.

Now, I have tested doing this and it does solve this specific problem.  I am concerned though that there are other cases where we could call ClearCachedInheritedStyleDataOnDescendants without realising that some frames still have a style context with cached structs that need to stick around so that CalcStyleDifference does the right comparisons.

I added some DEBUG-only tracking of whether a style context is being used by a frame, and added an assertion in DoClearCachedInheritedStyleDataOnDescendants that the style context we're clearing cached struct pointers on has no frames referencing it.  This sometimes asserts, so I'll have to look into that.

(It would be good if we didn't need ClearCachedInheritedStyleDataOnDescendants at all.  David we can try to work this out in a couple of weeks.)

Comment 2

[Cameron McCormack (:heycam)]

By the way, it is safe to leave those structs on the descendants of the old style contexts for that little while longer, as the <span>'s new style context will keep a hold of them until the end of the restyle process.

Comment 3

[Cameron McCormack (:heycam)]

Try push with the suggested fix and the assertion added, showing a whole bunch of assertion failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=211fdf876531

Comment 4

[Cameron McCormack (:heycam)]

It's tricky to get a useful assertion in DoClearCachedInheritedStyleDataOnDescendants, actually, since there are cases where we leave an old style context on a frame -- for example when we're going to reconstruct its frame -- and so we don't want to count those references as ones we care about.  We could move the ClearCachedInheritedStyleDataOnDescendants calls further up, into ComputeAndProcessStyleChange just after the ProcessRestyledFrames call (so that the frame reconstructions have been done), but I would worry then that somewhere in ProcessRestyledFrames we might somehow end up releasing the new style context that now has the owning reference to the style structs.

Comment 5

[Cameron McCormack (:heycam)]

Moving the ClearCachedInheritedStyleDataOnDescendants calls up to just after ProcessRestyledFrames clears up almost all of the assertion failures:

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

Comment 6

[Cameron McCormack (:heycam)]

The one assertion failure that seemed legitimate in the above try run is the one on layout/reftests/printing/1108104.html (the others are timeouts due to the nsStyleContext destructor expensive checks I enabled).  But I think this 1108104.html assertion failure has the same underlying cause as the existing 2 assertion failures in that test (which say that some style contexts are left alive with an old rule tree that should be disappearing).  The cause is that the replicated placeholder frame on the second page of the document never gets reconstructed even though an nsChangeHint_ReconstructFrame change is generated for it.  There must be something about the frame reconstruction code we call into from ProcessRestyledFrames that doesn't get to that placeholder.

I'll go ahead with this patch to call ClearCachedInheritedStyleDataOnDescendants after processing the change hints, and I'll leave this new assertion in too.

Comment 7

[Cameron McCormack (:heycam)]

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

Another concern I have with this approach is how big the mContextToClear array will grow within a single processed restyle.  Browsing a couple of news sites, I got a 631 element array.  We could do something like checking if this old style context's parent got added to the array (with the same or a superset of struct bits) and avoid adding it if so.

Comment 8

[Cameron McCormack (:heycam)]

Though note that 631 isn't necessarily the number of style contexts we call nsStyleContext::ClearCachedInheritedStyleDataOnDescendants on, since we still only do that if we detect it has at least one other strong reference, and by this point it is usual for them not to have any more strong references.

Comment 9

[Cameron McCormack (:heycam)]

Another example of a reason we might have style contexts alive with more than one strong reference at the time we decide to call ClearCachedInheritedStyleDataOnDescendants on them is that nsTransformedTextRun stores a strong reference to a style context per character of the text.  Although we might give a text frame a new style context, we don't clear its mTextRun until we reflow the text frame, so it can still keep the style context alive until we do reflow (or the frame goes away).  Also I guess that the global text run cache could hold on to the nsTransformedTextRuns too.

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #7)
> Another concern I have with this approach is how big the mContextToClear
> array will grow within a single processed restyle.  Browsing a couple of
> news sites, I got a 631 element array.  We could do something like checking
> if this old style context's parent got added to the array (with the same or
> a superset of struct bits) and avoid adding it if so.

Doing that reduced this particular case to 315 elements.

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

I think fixing bug 1067755 by not storing style contexts on transformed text runs will help further.

Comment 11

[Cameron McCormack (:heycam)]

The WIP patch I just posted to bug 1067755 substantially reduces the number of times we need to store any style contexts in mContextsToClear.

Comment 12

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

Comment 13

[Cameron McCormack (:heycam)]

As an example, loading cnn.com with this WIP patch and the bug 1067755 patch applied shows me this output:

ClearCachedInheritedStyleDataOnDescendants: 1/42
ClearCachedInheritedStyleDataOnDescendants: 1/1
ClearCachedInheritedStyleDataOnDescendants: 1/6
ClearCachedInheritedStyleDataOnDescendants: 0/38
ClearCachedInheritedStyleDataOnDescendants: 0/2
ClearCachedInheritedStyleDataOnDescendants: 1/47
ClearCachedInheritedStyleDataOnDescendants: 1/46
ClearCachedInheritedStyleDataOnDescendants: 0/6
ClearCachedInheritedStyleDataOnDescendants: 0/4
ClearCachedInheritedStyleDataOnDescendants: 0/10
ClearCachedInheritedStyleDataOnDescendants: 5/60
ClearCachedInheritedStyleDataOnDescendants: 0/48
ClearCachedInheritedStyleDataOnDescendants: 0/24
ClearCachedInheritedStyleDataOnDescendants: 0/65
ClearCachedInheritedStyleDataOnDescendants: 0/1
ClearCachedInheritedStyleDataOnDescendants: 0/1
ClearCachedInheritedStyleDataOnDescendants: 0/1
ClearCachedInheritedStyleDataOnDescendants: 0/1
ClearCachedInheritedStyleDataOnDescendants: 0/1
ClearCachedInheritedStyleDataOnDescendants: 0/1
ClearCachedInheritedStyleDataOnDescendants: 0/1
ClearCachedInheritedStyleDataOnDescendants: 1/21
ClearCachedInheritedStyleDataOnDescendants: 0/1

The number before the slash is the number of style contexts we call nsStyleContext::ClearCachedStructsOnDescendants on, which is how many have a single strong reference left at just after the ProcessRestyledFrames call of a single restyle.  The number after the slash is the length of mContextToClean at that point.  So most of the time we don't end up needing to call the struct clearing function on the style contexts, but up in ElementRestyler::Restyle we don't know that it is safe not to add them to mContextToClean.

Comment 14

[Cameron McCormack (:heycam)]

While I'd like to look into reducing the remaining cases (both instances where we end up calling nsStyleContext::ClearCachedStructsOnDescendants, and instances where we initially make the decision to append to mContextsToClean), I think we should get this in and turn the optimisation back on.  We can keep refining it later.  We should be doing much less work than when bug 931668 was turned on previously, when we were unconditionally calling nsStyleContext::ClearCachedStructsOnDescendants on old style contexts after structs were swapped away from them.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add a DEBUG-only count of frames that use a style context.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Part 2: Assert if we clear cached structs on a style context still used by a frame.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 3: Crashtest.

Comment 18

[Cameron McCormack (:heycam)]

Attachment: Part 4: Clear cached structs only after we have fully processed a restyle.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking this followup to bug 1092363 for 36 and later.

Comment 20

[Daniel Veditz [:dveditz]]

Is this a security issue in its own right or simply hidden because bug 1092363 was?

Comment 21

[Cameron McCormack (:heycam)]

It is the same issue that caused us to disable bug 931668 on current branches >= beta, so it is not a new issue, but I hid this bug because we wontfixed bug 1092363 on release.

Comment 22

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

Comment on attachment 8558142 [details] [diff] [review]
Part 1: Add a DEBUG-only count of frames that use a style context.

nsButtonFrameRenderer::SetStyleContext should have a NS_NOTREACHED
in a default case in the switch.

nsStyleContext.h should probably have comments adjacent to the three added
methods explaining what they're for.

~nsStyleContext should assert that mFrameRefCnt is 0.

r=dbaron with that

Comment 23

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

Comment on attachment 8558143 [details] [diff] [review]
Part 2: Assert if we clear cached structs on a style context still used by a frame.

It might be worth adding a comment explaining why
DoClearCachedInheritedStyleDataOnDescendants can't be called on a
style context still referenced by a frame.

r=dbaron

Comment 24

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

Comment on attachment 8558145 [details] [diff] [review]
Part 3: Crashtest.

I'm a little worried that in the future we might optimize away some of
the work associated with the .x {} empty rule.  Maybe make it non-empty
if it still meets the conditions for which it's a crashtest?

(Also maybe put newlines inside the style element while you're there.)

r=dbaron

Comment 25

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

OK, let's see if I can remember how we got here.

The basic optimization added in bug 931668 is that, in some cases, for
style changes that don't require rerunning selector matching on
descendants, we try to keep the old style contexts on descendants.  To
do this, we notice that at some point in the resolution process we have
a new style context that's equivalent to the old style context, and we
just keep the old style context, based on a set of conditions showing
it's safe to do so, and then skip rebuilding style contexts for
descendants.

In order to increase the cases in which this is a valid optimization, we
try, in the cases where we can't just use the old style context, keeping
as many of the old style structs as we can (when structs are cached on
the style context), by swapping structs (nsStyleContext::SwapStyleData)
between new and old (hopefully to-be-destroyed) context.  (This helps
the optimization because the descendant might inherit that struct, and
if they've inherited an old struct that we haven't swapped out, then we
wouldn't be able to do the optimization.)

This gets complicated because the old context for the element might not
actually be about to be destroyed; it might be sticking around for other
elements, thanks to sibling-sharing (which can recursively become
cousin-sharing, etc.).  To avoid that problem, we don't do the
struct-swapping if either the old or new style context is shared.

Then at some point you added ClearCachedInheritedStyleDataOnDescendants.
What was the motivation for that?  So that instead of bailing out of the
optimization, we could make the optimization, and then do some more
limited work on the descendants?

In any case, the logic of the fix seems good.  Part 4 prevents the
assertions from part 2 from firing in the case where you were seeing the
crash, right?  And hitting those assertions definitely seems bad in the
sense that it could cause us to drop style hints on the floor.  Was
dropping style hints on the floor what led to the crash?

One thing I'm curious about:
How does IsShared() test differ from the test that frame reference count
being greater than 1?  Could they be consolidated, or asserted to be
equivalent?

Comment 26

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

Comment on attachment 8558149 [details] [diff] [review]
Part 4: Clear cached structs only after we have fully processed a restyle.

One thing I wonder is whether it would make more sense to have the array
on the TreeMatchContext.  Would that simplify things?  I don't have a
particularly strong opinion.  (It feels a little bit wrong in that the
tree match context feels more like input to the process of matching a
tree, or caches used during it, rather than data carried around within
the process... but it might be simpler?)

My biggest concern, however, is that the call to
  ClearCachedInheritedStyleDataOnDescendants(contextsToClear);
is after the call to ProcessRestyledFrames.  If what this is doing
is clearing out dangling pointers, it seems like it has to be *before*
the call to ProcessRestyledFrames, since ProcessRestyledFrames can
certainly get style data from frames (e.g., by reflowing a subtree).

I'm a little worried about proving that nothing would access the
dangling pointers prior to that.  Is there a way you could add
assertions that would check that that isn't a problem?

Otherwise I think I'm happy with this.

Comment 27

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC+11) (needinfo? for questions) from comment #25)
> OK, let's see if I can remember how we got here.
> 
> The basic optimization added in bug 931668 is that, in some cases, for
> style changes that don't require rerunning selector matching on
> descendants, we try to keep the old style contexts on descendants.  To
> do this, we notice that at some point in the resolution process we have
> a new style context that's equivalent to the old style context, and we
> just keep the old style context, based on a set of conditions showing
> it's safe to do so, and then skip rebuilding style contexts for
> descendants.
> 
> In order to increase the cases in which this is a valid optimization, we
> try, in the cases where we can't just use the old style context, keeping
> as many of the old style structs as we can (when structs are cached on
> the style context), by swapping structs (nsStyleContext::SwapStyleData)
> between new and old (hopefully to-be-destroyed) context.  (This helps
> the optimization because the descendant might inherit that struct, and
> if they've inherited an old struct that we haven't swapped out, then we
> wouldn't be able to do the optimization.)
> 
> This gets complicated because the old context for the element might not
> actually be about to be destroyed; it might be sticking around for other
> elements, thanks to sibling-sharing (which can recursively become
> cousin-sharing, etc.).  To avoid that problem, we don't do the
> struct-swapping if either the old or new style context is shared.

This is all correct.  Sibling sharing isn't the only case, though; in general, style contexts could be held on to elsewhere.  Most of the cases I found were, as mentioned in comment 9, due to text run stuff.  I haven't had time yet to track down the remaining cases.

> Then at some point you added ClearCachedInheritedStyleDataOnDescendants.
> What was the motivation for that?  So that instead of bailing out of the
> optimization, we could make the optimization, and then do some more
> limited work on the descendants?

Yes.

> In any case, the logic of the fix seems good.  Part 4 prevents the
> assertions from part 2 from firing in the case where you were seeing the
> crash, right?

Yes.

> And hitting those assertions definitely seems bad in the
> sense that it could cause us to drop style hints on the floor.  Was
> dropping style hints on the floor what led to the crash?

Yeah, CalcStyleDifference didn't return the right hints because we assumed the nulled out cached struct pointers meant that we never fetched the relevant struct and thus we didn't need to actually compare it.

> One thing I'm curious about:
> How does IsShared() test differ from the test that frame reference count
> being greater than 1?  Could they be consolidated, or asserted to be
> equivalent?

It's not only other frames sharing the style context that cause them to be kept alive (as in comment 9).

(In reply to David Baron [:dbaron] (UTC+11) (needinfo? for questions) from comment #26)
> One thing I wonder is whether it would make more sense to have the array
> on the TreeMatchContext.  Would that simplify things?  I don't have a
> particularly strong opinion.  (It feels a little bit wrong in that the
> tree match context feels more like input to the process of matching a
> tree, or caches used during it, rather than data carried around within
> the process... but it might be simpler?)

It would simplify things in as much as we would have fewer pointers to copy down to each child ElementRestyler.  There are other things we copy straight down too -- mPresContext, mChangeList, mRestyleTracker, etc.  Maybe it would make sense to bundle these up into a single pointer; would we worry about the extra pointer dereference to look up any of these?

Alternatively we could keep the same amount of copying but just stick them into a non-pointer-valued member on ElementRestyler, and then we only need to copy one thing in the child-ElementRestyler constructors.

I could do that as a follow up if you like.

> My biggest concern, however, is that the call to
>   ClearCachedInheritedStyleDataOnDescendants(contextsToClear);
> is after the call to ProcessRestyledFrames.  If what this is doing
> is clearing out dangling pointers, it seems like it has to be *before*
> the call to ProcessRestyledFrames, since ProcessRestyledFrames can
> certainly get style data from frames (e.g., by reflowing a subtree).

The assertions I added rely on the call being done after ProcessRestyledFrames because otherwise we still have these about-to-be-reframed style contexts hanging around, keeping the mFrameRefCnt up.  If we could count them somehow, then we could check for that number in the DoClearCachedInheritedStyleDataOnDescendants assertion.

Or we could just set a bit (like the existing NS_STYLE_IS_GOING_AWAY) and skip the assertion on a style context with that bit, and hope that there are no frames using them that aren't going to be reframed.

> I'm a little worried about proving that nothing would access the
> dangling pointers prior to that.  Is there a way you could add
> assertions that would check that that isn't a problem?

We could recursively set a bit on the subtree that later will have ClearCached... called on it, then make the style struct getters assert that bit isn't present, and reset the bit once we do call ClearCached.


Let me know what you think of the above options.

Comment 28

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

(In reply to Cameron McCormack (:heycam) from comment #27)
> This is all correct.  Sibling sharing isn't the only case, though; in
> general, style contexts could be held on to elsewhere.  Most of the cases I
> found were, as mentioned in comment 9, due to text run stuff.  I haven't had
> time yet to track down the remaining cases.

Hmmm.  I'm not completely happy about other things holding on to style contexts, though I guess it's probably OK as long as they get released concurrently with the removal of those style contexts from the frames or the processing of change hints.  (Is that always the case, at least?)

> (In reply to David Baron [:dbaron] (UTC+11) (needinfo? for questions) from
> comment #26)
> > One thing I wonder is whether it would make more sense to have the array
> > on the TreeMatchContext.  Would that simplify things?  I don't have a
> > particularly strong opinion.  (It feels a little bit wrong in that the
> > tree match context feels more like input to the process of matching a
> > tree, or caches used during it, rather than data carried around within
> > the process... but it might be simpler?)
> 
> It would simplify things in as much as we would have fewer pointers to copy
> down to each child ElementRestyler.  There are other things we copy straight
> down too -- mPresContext, mChangeList, mRestyleTracker, etc.  Maybe it would
> make sense to bundle these up into a single pointer; would we worry about
> the extra pointer dereference to look up any of these?
> 
> Alternatively we could keep the same amount of copying but just stick them
> into a non-pointer-valued member on ElementRestyler, and then we only need
> to copy one thing in the child-ElementRestyler constructors.
> 
> I could do that as a follow up if you like.

Figuring this out as a followup is fine.

> > My biggest concern, however, is that the call to
> >   ClearCachedInheritedStyleDataOnDescendants(contextsToClear);
> > is after the call to ProcessRestyledFrames.  If what this is doing
> > is clearing out dangling pointers, it seems like it has to be *before*
> > the call to ProcessRestyledFrames, since ProcessRestyledFrames can
> > certainly get style data from frames (e.g., by reflowing a subtree).
> 
> The assertions I added rely on the call being done after
> ProcessRestyledFrames because otherwise we still have these
> about-to-be-reframed style contexts hanging around, keeping the mFrameRefCnt
> up.  If we could count them somehow, then we could check for that number in
> the DoClearCachedInheritedStyleDataOnDescendants assertion.
> 
> Or we could just set a bit (like the existing NS_STYLE_IS_GOING_AWAY) and
> skip the assertion on a style context with that bit, and hope that there are
> no frames using them that aren't going to be reframed.

I'm a little hesitant to weaken assertions here.

> > I'm a little worried about proving that nothing would access the
> > dangling pointers prior to that.  Is there a way you could add
> > assertions that would check that that isn't a problem?
> 
> We could recursively set a bit on the subtree that later will have
> ClearCached... called on it, then make the style struct getters assert that
> bit isn't present, and reset the bit once we do call ClearCached.

What about also temporarily keeping around the style contexts that the cached data that we're going to delete was inherited from?  (i.e., keeping around any style context that we reparent a style context from), or something like that?

(My concern, I think, was that we'd have dangling pointers that were data stored on those contexts, and use those dangling pointers after the style context was destroyed but before the ClearCached.)

Comment 29

[Cameron McCormack (:heycam)]

Attachment: Part 4: Clear cached structs only after we have fully processed a restyle. (v2)

Comment 30

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

Comment on attachment 8564814 [details] [diff] [review]
Part 4: Clear cached structs only after we have fully processed a restyle. (v2)

>+      // its original descendants, which are now owned by newParent (and which
>+      // might be destroyed before we call

I think "which" here should be "which otherwise", no?

The code in RestyleManager::ComputeAndProcessStyleChange could perhaps use a
comment explaining that oldStructOwners needs to live across
ProcessRestyledFrames but be destroyed before
ClearCachedInheritedStyleDataOnDescendants, and perhaps also why.

(The later repetitions could point to the first comment.)


r=dbaron with that

Comment 31

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

And, per discussion we just had, it's actually the new contexts that we want to hold in the array, since we're clearing cached style data on descendants of the old context (those that might have been swapped to the new context).  I'm not actually even confident it's possible for it to be a problem, but I'm not convinced it's not.

Comment 32

[Cameron McCormack (:heycam)]

This is the updated comment I'm adding:

      // We keep strong references to the new parent around until the end
      // of the restyle, in case:
      //   (a) we swapped structs between the old and new parent,
      //   (b) some descendants of the old parent are not getting restyled,
      //   (c) something under ProcessPendingRestyles looks at a cached
      //       struct on one of those descendants (where the struct is now owned
      //       by the new parent), and
      //   (d) something under ProcessPendingRestyles (which notably is called
      //       *before* ClearCachedInheritedStyleDataOnDescendants is called
      //       on the old context) causes the new parent to be destroyed, but
      //       then wants to look at one of those cached structs on an old style
      //       context's descendants.

Comment 33

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

(In reply to Cameron McCormack (:heycam) from comment #32)
> This is the updated comment I'm adding:
> 
>       // We keep strong references to the new parent around until the end
>       // of the restyle, in case:
>       //   (a) we swapped structs between the old and new parent,
>       //   (b) some descendants of the old parent are not getting restyled,
>       //   (c) something under ProcessPendingRestyles looks at a cached
>       //       struct on one of those descendants (where the struct is now
> owned
>       //       by the new parent), and
>       //   (d) something under ProcessPendingRestyles (which notably is
> called
>       //       *before* ClearCachedInheritedStyleDataOnDescendants is called
>       //       on the old context) causes the new parent to be destroyed, but
>       //       then wants to look at one of those cached structs on an old
> style
>       //       context's descendants.

Seems like the second half of (d) is the same as (c), no?  Maybe put (c) after (d) and use only the first half of (d)?

Also maybe mention in (b) that having such descendants is the reason we have ClearCachedInh...?

Comment 34

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC+13) (needinfo? for questions) from comment #33)
> Seems like the second half of (d) is the same as (c), no?  Maybe put (c)
> after (d) and use only the first half of (d)?

Oh, so it is.

Comment 35

[Cameron McCormack (:heycam)]

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

Comment 36

[Cameron McCormack (:heycam)]

Comment on attachment 8564814 [details] [diff] [review]
Part 4: Clear cached structs only after we have fully processed a restyle. (v2)

(This sec-approval request is for all four patches.)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't think it would be easy based on the patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The crashtest suggests that text-decoration is involved and given the assertions in part 1, this might help someone start investigating this.  It's not a bulls-eye though.

Which older supported branches are affected by this flaw?

35 (current Release) is the only branch affected.

If not all supported branches, which bug introduced the flaw?

bug 931668

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

We wontfixed the bug on 35 in bug 1092363, so I'm not proposing to land this more complex solution there.  The patches don't apply cleanly to Aurora but it shouldn't be hard to fix that.

How likely is this patch to cause regressions; how much testing does it need?

Not likely, small chance I guess.  I plan to land this on current Aurora and Central (or Beta, Aurora and Central, if sec-approval- and we wait until 36, which isn't affected, is Release).

Comment 37

[Al Billings [:abillings - ex-MoCo]]

This is too late to make the 36 release. We need a security rating on this bug before we can approve it elsewhere. Since bug 1092363 is a sec-high, should I assume that this is as well?

Comment 38

[Cameron McCormack (:heycam)]

Yeah sec-high is appropriate.

Comment 39

[Al Billings [:abillings - ex-MoCo]]

Ok. So this issue is disabled in 36 (from reading comments). Is this correct?

If so, I can approve for Trunk and Aurora.

Comment 40

[Cameron McCormack (:heycam)]

That's right.  If we waited until 36 became Release, then I wouldn't need to request sec-approval.  Better to get it into Aurora now than Beta in a week, though.

Comment 41

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1b8b9ec10a
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd451e36fd91
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac22d8f07fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d2f893feb9

https://hg.mozilla.org/releases/mozilla-aurora/rev/883c0f7c2a38
https://hg.mozilla.org/releases/mozilla-aurora/rev/f3372cc7e767
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf7a3467b5a6
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b95c24edb89

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/883c0f7c2a38
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f3372cc7e767
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/bf7a3467b5a6
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4b95c24edb89

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a1b8b9ec10a
https://hg.mozilla.org/mozilla-central/rev/dd451e36fd91
https://hg.mozilla.org/mozilla-central/rev/1ac22d8f07fa
https://hg.mozilla.org/mozilla-central/rev/60d2f893feb9

Comment 44

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

Is it worth filing followups on either (a) comment 14 or (b) the followup bit of comment 28?

Comment 45

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #44)
> Is it worth filing followups on either (a) comment 14

At this point I'd rather work on bug 1188721 to make style structs refcounted, which would make all of this mContextsToClear processing go away.

> (b) the followup bit of comment 28?

I filed bug 1213707.