1067755 - When resizing browser at Nightly firstrun page, several copies of "###!!! ASSERTION: old rule tree still referenced: 'Not Reached', file $SRCDIR/layout/style/nsStyleSet.cpp, line 2076"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Visit Nightly firstrun page in a debug build:
 https://www.mozilla.org/en-US/firefox/nightly/firstrun/

 2. Resize your browser window's width to be as small as you can, and then make it normal-sized again.

ACTUAL RESULTS:
Several copies of this assertion are spammed when the page snaps across a media-query boundary (at around 800 pixels wide):
{
###!!! ASSERTION: old rule tree still referenced: 'Not Reached', file $SRCDIR/layout/style/nsStyleSet.cpp, line 2076
}

See also bug 1060194, filed on this same assertion in Firefox OS. (There, it "keeps showing without doing anything" -- may or may not be the same underlying bug as what's happening here at the firstrun page.)

Comment 1

[Daniel Holbert [:dholbert]]

Sorry, I actually get two separate assertions.  When my browser window crosses the ~800px threshold (and the layout snaps to a skinnier design), I first get two copies of this assertion
{
###!!! ASSERTION: style context has old rule node: 'n == mRuleTree', file $SRCDIR/layout/style/nsStyleSet.cpp, line 247
}
...and *then* I get 2 copies of the "old rule tree still referenced" NS_NOTREACHED from comment 0.

Both of these assertions were added in bug 475128 (mozilla-central changeset 3b2f69cc7004); hence, adding dependency on that bug. (not to imply that this started with that bug's landing, but just that it's related and the problem wouldn't have been detectable without that bug's commit)

Comment 2

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

I've debugged what's going on here; more details after a meeting.

Comment 3

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

I think this is probably a regression from bug 950526, although I haven't verified this.  (I was originally expecting it to be a regression from bug 931668, but I don't think that's the case.)

The underlying problem here is that we have an nsTransformedTextRun that's holding on (in its mStyles) to a style context.  We then do a restyle that rebuilds the rule tree, which is supposed to get rid of all style contexts that refer to the old rule tree and replace them with ones that point to the new rule tree.  We assert at the end of that restyle that there are no style contexts referencing the old rule tree, and this assertion fails because the nsTransformedTextRun is still holding them.

Comment 4

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

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #3)
> I think this is probably a regression from bug 950526, although I haven't
> verified this.  (I was originally expecting it to be a regression from bug
> 931668, but I don't think that's the case.)

It's at least a regression in the sense that adding the ClearTextRuns() call back to nsTextFrame::DidSetStyleContext makes this assertion go away.  (I'm not sure that there wasn't a more recent change involved as well.)

Ideally we should find a way for nsTransformedTextRun to either:
 (1) not hold style contexts, but instead hold the information it needs (doesn't look too hard except for MathML!), or
 (2) update the style context pointers in the nsTransformedTextRun when needed.

Comment 5

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

Attachment: simplified testcase

Comment 6

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

The assertions I needed to add to debug this are in bug 1075082.

Comment 7

[Cameron McCormack (:heycam)]

John, I want to do (1) from comment 4.  I'm thinking to replace the array of refcounted nsStyleContext pointers on transformed text runs with pointers to new objects that just encapsulated the required style from those contexts.

This simplest approach is probably to have a single new struct, nsTransformedCharStyle, which has members for all possible style data needed by the transformers for a given character, regardless of which transforming text run factories we use.  This wouldn't be very generic (unlike the current setup that allows a chain of transforming factories).

Alternatively, we could do something like have nsTransformedCharStyle be an object with a table of transformer-specific styles.  We would have BuildTextRunsScanner::BuildTextRunForFrames call into the factory chain to build up this array of nsTransformedCharStyle objects with the right transformer-specific styles.  In this setup we should probably just stick the current values required by the case transforming factory -- mTextTransform and mLanguage -- on nsTransformedCharStyle itself, and leave the table just for the MathML stuff.

Do you have a preference?

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch

This is approach (a).

Comment 9

[Cameron McCormack (:heycam)]

The "simplest approach", that is (I thought I lettered the two approaches).

Comment 10

[Cameron McCormack (:heycam)]

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

Comment 11

[John Daggett (:jtd)]

(In reply to Cameron McCormack (:heycam) from comment #7)

> This simplest approach is probably to have a single new struct,
> nsTransformedCharStyle, which has members for all possible style data needed
> by the transformers for a given character, regardless of which transforming
> text run factories we use.  This wouldn't be very generic (unlike the
> current setup that allows a chain of transforming factories).

This seems fine. I would run it by roc also.

Comment 12

[Cameron McCormack (:heycam)]

roc is OK with this approach too.

Comment 13

[John Daggett (:jtd)]

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

Review of attachment 8557853 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming the rvalue reference usage is correct, looks good.

Comment 14

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cab2c930fa02

Comment 15

[Carsten Book [:Tomcat]]

sorry had to back this out for static analysis bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=6453305&repo=mozilla-inbound

Comment 16

[Cameron McCormack (:heycam)]

Thanks, I was just missing an explicit.

https://hg.mozilla.org/integration/mozilla-inbound/rev/87b7034309e2

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87b7034309e2