383979 - "ASSERTION: Some frame destructors were not called" with font properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase (reload or close to see the assertion)

###!!! ASSERTION: Some frame destructors were not called: 'mFrameCount == 0', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 674

The testcase is surprisingly simple.  The strangest part is the "font" shorthand: to eliminate it from the testcase, I have to replace it with *seven* separate properties:

  font-family: arial;
  font-style: normal;
  font-variant: normal;
  font-weight: normal;
  font-size: 8pt;
  font-size-adjust: none;
  text-decoration: underline;

Why are all those "normal" and "none" properties needed in order to trigger the bug?

Comment 1

[Nickolay_Ponomarev]

The leaked thing is an nsStyleFont, created at the following stack:

nsRuleNode::ComputeFontData(aStartStruct=0x050af7cc, aData={...}, aContext=0x050af2ec, aHighestNode=0x04334e1c, aRuleDetail=eRulePartialReset, aInherited=0x00000000)  Line 2360	C++
nsRuleNode::WalkRuleTree(aSID=eStyleStruct_Font, aContext=0x050af2ec, aRuleData=0x0012906c, aSpecificData=0x00129024)  Line 73	C++
nsRuleNode::GetFontData(aContext=0x050af2ec)  Line 1171	C++
nsRuleNode::GetStyleFont(aContext=0x050af2ec, aComputeData=0x00000001)  Line 73	C++
nsStyleContext::GetStyleFont()  Line 73	C++
nsRuleNode::ComputeFontData(aStartStruct=0x050af7cc, aData={...}, aContext=0x050af374, aHighestNode=0x04334e1c, aRuleDetail=eRulePartialReset, aInherited=0x00000000)  Line 2283	C++
nsRuleNode::WalkRuleTree(aSID=eStyleStruct_Font, aContext=0x050af374, aRuleData=0x001292a0, aSpecificData=0x00129258)  Line 73	C++
nsRuleNode::GetFontData(aContext=0x050af374)  Line 1171	C++
nsRuleNode::GetStyleData(aSID=eStyleStruct_Font, aContext=0x050af374, aComputeData=0x00000001)  Line 73	C++
nsStyleContext::GetStyleData(aSID=eStyleStruct_Font)  Line 224	C++
nsRuleNode::WalkRuleTree(aSID=eStyleStruct_Font, aContext=0x04176990, aRuleData=0x001293c4, aSpecificData=0x0012937c)  Line 1461	C++
nsRuleNode::GetFontData(aContext=0x04176990)  Line 1171	C++
nsRuleNode::GetStyleFont(aContext=0x04176990, aComputeData=0x00000001)  Line 73	C++
nsStyleContext::GetStyleFont()  Line 73	C++
nsIFrame::GetStyleFont()  Line 73	C++
BuildTextRunsScanner::BuildTextRunForFrames(aTextBuffer=0x0012a7b4)  Line 1551	C++
BuildTextRunsScanner::FlushFrames(aFlushLineBreaks=0x00000001)  Line 1248	C++
BuildTextRuns(aRC=0x04192aa8, aForFrame=0x050af1e4, aLineContainer=0x050af124, aForFrameLine=0x0012c7a4)  Line 1207	C++
nsTextFrame::EnsureTextRun(aRC=0x04192aa8, aLineContainer=0x050af124, aLine=0x0012c7a4, aFlowEndInTextRun=0x0012bde8)  Line 1953	C++
nsTextFrame::Reflow(aPresContext=0x043641c0, aMetrics={...}, aReflowState={...}, aStatus=0x00000000)  Line 5266	C++

As far as I can see, what happens is: the style contexts for #s1 and #s2 share the same nsRuleNode, so the last two ComputeFontData calls happen with the same |this| value. The second ComputeFontData in the stack above overwrites the nsStyleFont that was assigned to aHighestNode's style data by the first ComputeFontData.

One other consequence is that things like 'font-size: larger' in that rule appear to have twice the expected effect.

Not sure how this should be fixed though.


> Why are all those "normal" and "none" properties needed in order to trigger the
> bug?

Because per http://www.w3.org/TR/CSS21/fonts.html#font-shorthand the |font| property sets the font-related properties that are not explicitly set to their defaults.

Comment 2

[Jesse Ruderman]

>> Why are all those "normal" and "none" properties needed in order to trigger the
>> bug?

>Because per http://www.w3.org/TR/CSS21/fonts.html#font-shorthand the |font|
>property sets the font-related properties that are not explicitly set to their
>defaults.

That doesn't answer my question...

Comment 3

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

If text-decoration weren't needed, I'd guess because that way all the properties in the struct are specified.

Comment 4

[Jesse Ruderman]

I'm curious why I need seven font properties to trigger the bug; why can't six trigger it?  Is the key that a struct has no inherited parts?

Comment 5

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

This one is fun.  I think I see what's happening.

There's an underlying bug in the COMPUTE_START_INHERITED preamble, where it calls parentContext->GetStyle##type_ in cases where it might store the data on the rule node rather than the context.  That's the basic cause of the leak.

But the testcase needed to cause that would normally be even more convoluted than this one.  The reason you've triggered it in this simpler case is because of a problem in CheckFontCallback introduced by my system font rewrite.

And while I'm in CheckFontCallback, I've noticed it doesn't handle LARGER and SMALLER correctly.

I want to write separate testcases for these other bugs (although I'm not sure there's a testcase other than this one for the main CheckFontCallback problem), and make sure I'm thinking this through correctly.

Comment 6

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

Attachment: testcase triggering same assertion/leak using nsStyleList

Comment 7

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

Note that s2 and s3 need to be parent/child (and have the same rule node), but s1 need not be the parent of s2 and s3.

I actually don't think it's really a difference in complexity, and I don't think it's related to my system font rewrite (in fact, you probably filed this before it), although I did introduce a bug there that I probably should fix.

Comment 8

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

Attachment: patch

This fixes the basic problem in Compute*Data, which is that we should not call GetStyle##type_ on the parent context if we could potentially store our data on the rule node, since if the parent context has the same rule node, the two recursive Compute*Data functions will overwrite each other's storing the struct on the rule node as they unwind.  The one case where that could happen is when we had aRuleDetail == eRulePartialReset combined with a non-null aStartStruct.  (It would happen for aRuleDetail == eRuleNone combined with a non-null aStartStruct too, except the assertion documents that that never happens.)

It also adds a few assertions and removes an "XXXldb" comment that the assertion makes clear is bogus.

I'll be filing some followup bugs on the other things I discovered.

Comment 9

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

(In reply to comment #1)
> One other consequence is that things like 'font-size: larger' in that rule
> appear to have twice the expected effect.

... which is actually one of the other bugs I noticed while reading the code.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

>+  NS_ASSERTION(aRuleDetail != eRuleFullInherited &&                           \
>+               aRuleDetail != eRulePartialInherited &&                        \
>+               aRuleDetail != eRuleNone,                                      \
>+               "should not have bothered calling Compute*Data");              \

I'm trying to understand why we can assert this.  The only cases when we avoid calling Compute##name##Data in nsRuleNode::WalkRuleTree are:

- detail == eRuleNone && startStruct && !aRuleData->mPostResolveCallback

- startStruct && ((!isReset && (detail == eRuleNone || detail == eRulePartialInherited)) 
                              || detail == eRuleFullInherited)

(I think we should add an assert before that second test that "!aStartStruct || (detail != eRuleFullInherited && detail != eRuleFullMixed && detail != eRuleFullReset)", and slightly simplify the if condition there: the last check could hang out on its own, or'ed with the rest of it).

In any case, none of that enforces the conditions you specify above.  In particular, if aDetail == eRulePartialInherited and we have a start struct, we will need to compute data.  Not to mention the post-resolve callback mess, which really doesn't seem to be that consistently handled here anyway.  I do think if we made the change to the second condition I describe above, we could assert that detail != eRuleFullInherited.

Or am missing something here?

>+  /* If |inherited| might by false by the time we're done, we can't call */   \

"might be".

>+  /* parentContext->GetStyle##type_() since it could recur into setting */    \
>+  /* the same struct on the same rule node, causing a leak. */                \
>+  if (parentContext && aRuleDetail != eRuleFullReset &&                       \
>+      (!aStartStruct || aRuleDetail != eRulePartialReset))                    \
>     parentdata_ = parentContext->GetStyle##type_();                           

So we're assuming that if we're fully-specified with only reset values, we know we won't need the parent data?  None of the computation stuff (even for fonts) uses it if everything is reset?  I'm looking at the generic == gGenericNone case here, where we look at parentFont->mFlags.

For that matter, nsRuleNode::SetGenericFont itself will call GetStyleFont() on parent contexts, so it could also get us into trouble, no?

> #define COMPUTE_START_RESET(type_, ctorargs_, data_, parentdata_, rdtype_, rdata_) \
>+  NS_ASSERTION(aRuleDetail != eRuleFullInherited,                             \
>+               "should not have bothered calling Compute*Data");              \

This assert I can buy if we make my proposed changes to WalkRuleTree.

>+  /* If |inherited| might by false by the time we're done, we can't call */   \

Again, "might be".

I'd like to see clarifications on those asserts and on SetGenericFont.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

r- per comments

Comment 12

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

So I don't think the proposed changes to WalkRuleTree actually change anything, but the assertion and simplification of the condition are good, so I'll do them.  And you're right about the eRulePartialInherited and eRuleNone cases.

I don't think SetGenericFont is an issue because inherited is forced to PR_TRUE whenever we call SetGenericFont.  That said, I'd like to remove SetGenericFont for other reasons, but there's another bug on that somewhere...

Comment 13

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

Attachment: patch

See previous comment.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> So I don't think the proposed changes to WalkRuleTree actually change anything,

Right.  I think they just make the code clearer.

Will look at patch ASAP.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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


It seems to me that all codepaths that end up actually using the parent struct need to:

1)  Set |inherited| to true (I think we already do that)
2)  Make sure that the parent struct is really the parent struct.  I'm not sure
    this patch does that.

I guess the real point is that the parent struct might end getting used even if aRuleDetail is eRuleFullReset, say, in the current code.  Certainly for the Font struct, where all codepaths (generic and not) always rely on the parent struct.

I seem to recall us planning to fix this business about different font sizes for monospace fonts... that would help here, right?

Or is the mFlags thing ok because in the fully-specified case we really don't want to be looking at the parent font (or because in that case the default font doesn't matter)?

r+sr=me assuming the font business is sorted out in terms of what remains to be done to make it work.  I'm just hoping we don't have any other structs that behave that way (actually need the parent struct in cases when the rules are fully specified).

Comment 16

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

Oh, the mFlags test in ComputeFontData itself isn't OK (and wasn't in the past, either).  I should probably just fix the "XXXldb" comment there.

Comment 17

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

And, for reference, bug 216456 has some other improvements to SetGenericFont (awaiting review), and bug 380915 is the bug on getting rid of the whole mess.

Comment 18

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

Attachment: patch

SetGenericFont is ok because it explicitly deals with the parent context, and because it sets inherited to true.

However, the GetDefaultFont in the generic == kGenericFont_NONE case was broken, and I fixed it by fixing a test that I'm pretty confident was bogus anyway.

I also audited for uses of the parent* variables and found only one problem:  font-weight: smaller and larger.  This patch also fixes those.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Looks great.

Comment 20

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

Fix checked in to trunk.

Comment 21

[Jesse Ruderman]

Crashtests checked in.