385354 - Hang with ::before text, text-align: right and white-space: -moz-pre-wrap

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

I guess this might be similar as bug 385344.
This regressed when the new textframe patch landed.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is an interesting one. We seem to be building a bad frame tree:

              Block(div)(1)@0x25e2444 next=0x25e295c {0,0,4732,2304} [state=00001000] sc=0x25e2264(i=3,b=0)<
                line 0x25e28ec: count=2 state=inline,dirty,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x40a1] {1440,0,3292,1152} ca={0,0,4732,1152} <
                  Text(0)@0x25e23b8[0,1,T]  next=0x25e25a4 {1440,96,0,960} [state=08200000] [content=0x3d32fb40] sc=0x25e249c pst=:-moz-non-element<
                    "\n"
                  >
                  Inline(object)(1)@0x25e25a4 next=0x25e2c4c next-continuation=0x25e2c4c {1440,96,3292,960} [state=00000400] [content=0x3d0f7510] [sc=0x25e2554]<
                    Inline(object)(1)@0x25e26ac next=0x25e27c8 next-continuation=0x25e2cf0 {0,0,3292,960} [state=00000040] [content=0x3d0f7510] [sc=0x25e2604] pst=:before<
                      Text(-1)@0x25e24ec[0,7,F]  next-continuation=0x25e2cac {0,0,3292,960} [state=00600040] [content=0x3d35a620] sc=0x25e26e4 pst=:-moz-non-element<
                        "before "
                      >
                    >
                    Inline(span)(0)@0x25e27c8 {0,0,0,0} [state=00000402] [content=0x3d331100] [sc=0x25e279c]<
                      Text(0)@0x25e2734[0,1,T]  {0,0,0,0} [state=00000402] [content=0x3d359e50] sc=0x25e285c pst=:-moz-non-element<
                        "\n"
                      >
                    >
                  >
                >
                line 0x25e2c84: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x2881] {2692,1152,2040,1152} ca={0,1152,4732,1152} <
                  Inline(object)(1)@0x25e2c4c next=0x25e28ac prev-continuation=0x25e25a4 {2692,1248,2040,960} [state=00000404] [content=0x3d0f7510] [sc=0x25e2554]<
                    Inline(object)(1)@0x25e2cf0 prev-continuation=0x25e26ac {0,0,1440,960} [state=00000044] [content=0x3d0f7510] [sc=0x25e2604] pst=:before<
                      Text(-1)@0x25e2cac[7,4,T]  prev-continuation=0x25e24ec {0,0,1440,960} [state=00200044] [content=0x3d35a620] sc=0x25e26e4 pst=:-moz-non-element<
                        "text"
                      >
                    >
                  >
                >
                line 0x25e2d5c: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x2180] {4732,2304,0,0} <
                  Text(2)@0x25e28ac[0,1,T]  {4732,2304,0,0} [state=08200000] [content=0x3d359e80] sc=0x25e249c pst=:-moz-non-element<
                    "\n"
                  >
                >
              >

Note that the span's \n is between "before" and "text" ... this definitely should not happen! Probably an existing bug that new-textframe has exposed.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix -w

The problem here is that when we change the white-space property on the span, its frame is reconstructed in the wrong place. The nsCSSFrameConstructor has code to account for :before frames when inserting the first child of a node, but that code doesn't run when isAppend is true --- in this case, it needs to. The fix is simple.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: revised fix

oops, the previous patch set isAppend=PR_FALSE a little more than necessary.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: revised fix -w

Oops, the previous patch set isAppend to false a little more often than necessary.

Comment 6

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

Comment on attachment 269331 [details] [diff] [review]
revised fix -w

r+sr=dbaron, although I don't see why you need to set isAppend to false at all.  It might be good to add a comment explaining why.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suppose we could set appendAfterFrame instead and take the Append path. But I think it's just as simple, and lower risk, to use the Insert path for all "insert after :before frames" situations. I'll add a comment to mention that.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 9

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071905 Minefield/3.0a7pre

Comment 10

[Mats Palmgren (inactive)]

Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f4e6810b9a1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f4e6810b9a1