393325 - Reflow whenever ThemeSupportsWidget changes (was: "ASSERTION: Incorrect size computed by ComputeAutoSize?")

Status: NEW

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Incorrect size computed by ComputeAutoSize?: 'ancestorAutoSize.width == autoSize.width', file /Users/jruderman/trunk/mozilla/layout/forms/nsTextControlFrame.cpp, line 1743

The testcase uses SVG, but I'm pretty sure it's possible to trigger the assertion without SVG as well.

The assertion was added in bug 385870.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This worskforme, in the build that has that patch, on Linux.

Could this be Mac-specific?  Or dependent on native theming somehow?

What are your ancestorAutoSize and autoSize in this case?

Comment 2

[Jesse Ruderman]

(gdb) f 4
#4  0x17abdc69 in nsTextControlFrame::ComputeAutoSize (this=0x3eb5b210, aRenderingContext=0x3e3673a0, aCBSize=@0xbfffbb98, aAvailableWidth=30000, aMargin=@0xbfffbb90, aBorder=@0xbfffbb88, aPadding=@0xbfffbb80, aShrinkWrap=1) at /Users/jruderman/trunk/mozilla/layout/forms/nsTextControlFrame.cpp:1742
1742        NS_ASSERTION(ancestorAutoSize.width == autoSize.width,
(gdb) p ancestorAutoSize.width
$1 = 9060
(gdb) p autoSize.width
$2 = 8940
(gdb) p ancestorAutoSize.height
$3 = 1073741824
(gdb) p autoSize.height
$4 = 777

Comment 3

[Boris Zbarsky [:bzbarsky]]

Huh.  So I can reproduce this on Mac, but I get complete garbage in autoSize.width according to gdb...  I wonder what's going on.  :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the key here, I think, is that our computed border has changed but we didn't dirty our mPrefSize in the process.

The problem is, as far as I can tell, that our border-style (and color) has changed.  In particular, that change means that ThemeSupportsWidget starts returning false instead of true, and that therefore the value that nsBox::GetBorder returns changes.  But the style change only gives a repaint hint, so we never do a FrameNeedsReflow which would dirty our cached mPrefSize.

Then we try to compute our size based on the cached mPrefSize and the new border (which is 1px smaller), and of course this gives a different answer from the direct computation, triggering the assertion.  Ideally we'd also have asserts in place that mPrefSize is still correct (as in, recompute it in debug builds), but whatever.

Really, we need to do a reflow whenever ThemeSupportsWidget changes...  Question is how to detect that efficiently.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Perhaps we should actually store the "has author style" bit on the style context (and only compute it if moz-appearance is set)?  Then we'd only compute it once, and we could set a reflow hint if that bit differs from old context to new context...  David, what do you think?

Comment 6

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

Sounds good to me; it would address bug 240117 comment 9 as well.  It might be better to make the bit mean "is themed", though, since that would be easier to compute correctly in the normal case (default to 0).

I wonder if IsWidgetStyled could take an nsIContent* instead of an nsIFrame*.  That would help guarantee that this was safe.

Comment 7

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

And note that I think IsWidgetStyled could be called CanThemeWidget, or something like that, and the user of nsITheme (in layout) could be the ones checking style data.

And perhaps even a PRBool aIsHTML rather than nsIContent *aContent.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So the idea would be to replace the checks for mAppearance for checks of this style context bit in layout?  And remove all the style-checking from widget/ code altogether?

Comment 9

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

I think so, especially if the only thing nsITheme implementations do with the frame is check IsContentOfType(eHTML).  (It would mainly be changing nsIFrame::IsThemed.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

This causes real problems like bug 494507.

Comment 11

[B.J. Herbison]

Changing the backgroundColor of a select caused problems on Linux. Is that a separate issue or is the platform of this bug incorrect?

Comment 12

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

That is a separate bug.