420069 - PGO layout regression with horizontal lists of links

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Steve England [:stevee]]

Attachment: testcase to show layout differences with PGO enabled.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022714 Minefield/3.0b4pre ID:2008022714

Samson from the mz forum noticed this and provided the start of this testcase.

1. New profile, start firefox
2. Load testcase

Expected:
 - Mozilla   Mozilla   Mozilla   Mozilla   Mozilla   Mozilla

Actual:
 - MozillaMozillaMozillaMozillaMozillaMozilla

Works: 20080227_1215_firefox-3.0b4pre.en-US.win32
Broken: 20080227_1248_N_firefox-3.0b4pre.en-US.win32

Checkins to module PhoenixTinderbox between 2008-02-27 12:15 and 2008-02-27 12:47 : 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1204143300&maxdate=1204145279

The turning on off PGO lies within this range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1204143300&maxdate=1204145279&cvsroot=%2Fcvsroot

Comment 1

[Steve England [:stevee]]

Attachment: image of rendering difference

Comment 2

[Boris Zbarsky [:bzbarsky]]

When the bug appears, does DOM Inspector report the right specified style for the padding?  The right computed style? Would be nice to pin down whether it's CSS parsing, style computation, or layout that's being miscompiled...

Comment 3

[(not currently active) Ted Mielczarek]

See also bug 420092

Comment 4

[Mike Shaver (:shaver emeritus)]

Firebug reports that computed padding is 8px for each of top/right/bottom/left on the <a> element.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, so the issue is somewhere in the layout code...

Someone want to spin up a PGO-enabled debug build with the layout-debugger extension and see what the frame tree looks like?

Comment 6

[(not currently active) Ted Mielczarek]

I have debug symbols on this PGO build I just made, but I didn't --enable-debug. I'm out of luck there, aren't I?

My plan was to run this through TUnit and mochitest anyway.

Comment 7

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

For what it's worth, I suspect it's probably the same bug that's causing two of the text-indent reftests to fail in a PGO build.  We're misrendering the first case in mozilla/layout/reftests/text-indent/text-indent-intrinsic-pref-ref.html and mozilla/layout/reftests/text-indent/text-indent-intrinsic-min-ref.html by putting the margin-left on both sides of the span instead of just the left side.  (This is from bug 420092 comment 2; skip to bug 420092 comment 14 after reading that.)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, I think you need to --enable-debug and enable the layoutdebug extension (or whatever it's called) to get the frame dump.

Comment 10

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

The behavior is consistent with a bug where nsContainerFrame::DoInlineIntrinsicWidth is somehow crossing padding and margin and thus handling the margin twice and the padding not at all.

Comment 11

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

And now that I look at it, I think this code is probably invalid:

  nsStyleCoord tmp;

  // This goes at the beginning no matter how things are broken and how
  // messy the bidi situations are, since per CSS2.1 section 8.6
  // (implemented in bug 328168), the startSide border is always on the
  // first line.
  aData->currentLine +=
    GetCoord(stylePadding->mPadding.Get(startSide, tmp), 0) +
    styleBorder->GetBorderWidth(startSide) +
    GetCoord(styleMargin->mMargin.Get(startSide, tmp), 0);

since it's using tmp for both temporaries and assuming the result comes out of one before the other starts.

Comment 12

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

Attachment: possible patch

I bet this fixes it (though I don't have a setup to test; I haven't even run the patch yet, but it does compile).  The code that this patches was depending on undefined order-of-evaluation.

Comment 13

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

And in the longer term I think we should probably inline nsStyleCoord's copy constructor and then make nsStyleSides::Get* just return nsStyleCoords by value.

I should also add reftests for the various cases to test this function better; we only tested margin-left in current reftests.

Comment 14

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

Attachment: ... and fix the broken API

This applies on top of the previous patch, and fixes the broken API (from back when we had rules against returning structs by value).  Still want to run a few more tests on this one before requesting reviews, and I think it can wait until after beta4.

Comment 15

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

Oh, and while writing attachment 306396 [details] [diff] [review], I was paying attention to whether there were any more occurrences of the broken pattern.  I didn't find any.  All other uses seemed to be separated by ";", "||", or "&&".

Comment 16

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

Attachment: reftests

These cover the broken code better; I'll land them when the tree reopens.

Comment 17

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

Comment on attachment 306396 [details] [diff] [review]
... and fix the broken API

This passes reftests (except mozilla/layout/reftests/text/zwnj-02.html and mozilla/layout/reftests/bidi/386339.html, which don't seem at all related to this patch and I presume fail without it) and passes style system mochitests.

Comment 18

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

Comment on attachment 306353 [details] [diff] [review]
possible patch

Want to get the simple fix in for beta4.

Comment 19

[David Mandelin [:dmandelin]]

Attachment: Dehydra GCC script

I took a crack at making a Dehydra GCC script to detect bugs of this sort. The pattern I am looking for is an subexpression that does not contain internal sequence points that has a variable that is either (a) written more than once (as in this bug) or (b) written and read.

The script does detect this bug, but when I ran it on the rest of layout, all else I got was a few false positives. This may be evidence that there aren't other bugs of this sort, at least in layout.

Comment 20

[Damon Sicore (:damons)]

Comment on attachment 306353 [details] [diff] [review]
possible patch

a1.9b4+=damons

Comment 21

[Matt]

I assume this is causing weird layout issues I'm seeing on facebook.com?

Comment 22

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

Attachment 306353 [details] [diff] and attachment 306402 [details] [diff] [review] checked in to trunk, 2008-02-28 17:52 -0800.

Comment 23

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022901 Minefield/3.0b4pre ID:2008022901
Testcase now WFM.

Comment 24

[u88484]

Testcase WFM also XP 2008022906.  And an issue I saw on yahoo.com is also fixed that was the same bug as this.

Comment 25

[Peter van der Woude [:Peter6]]

Gmail tags alignment fixed aswell

Comment 26

[Roger Libiez]

The issue that prompted me to post in the build forums for www.iguanadons.net is fixed as well.

Comment 27

[Steve England [:stevee]]

--> VERIFIED per comments 23, 24, 25 and 26.

Comment 28

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/reftests/bugs&command=DIFF_FRAMESET&file=reftest.list&rev1=1.376&rev2=1.377&root=/cvsroot

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306396 [details] [diff] [review]
... and fix the broken API

a1.9=beltzner

Comment 30

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

Attachment 306396 [details] [diff] checked in to trunk, 2008-03-05 16:05 -0800.