330373 - [regression] Duplicated text in certain bidi case with image

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Uri Bernstein]

This is a regression from bug 329878 (why do I only find these after checking in?).

I'll attach a testcase immediately.
When loading the testcase for the first time, or shift-reloading it later, some the text from the second line is duplicated on the first line (which therefore overflows to the right of the green box). Doing a regular reload, or changing text size, fixes the problem.

This is apparently triggered by an incremental reflow, but not other kinds of reflows, which is why I haven't found it before. The in-flows which are "left alone" by EnsureBidiContinuation aren't being reused properly in this case, but just stay there.

I don't have a fix for this yet. If I won't find one, we might need to consider backing out bug 329878, and perhaps bug 319930 as well (since it basically does the same thing).

Comment 1

[Uri Bernstein]

Attachment: testcase

Description is in comment #0.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> The in-flows which are "left alone" by EnsureBidiContinuation aren't being
> reused properly in this case, but just stay there.

Should the relevant lines be getting marked dirty or something?

Comment 3

[Uri Bernstein]

(In reply to comment #2)
> > The in-flows which are "left alone" by EnsureBidiContinuation aren't being
> > reused properly in this case, but just stay there.
> 
> Should the relevant lines be getting marked dirty or something?
> 

So, what happens here is that only the second (and third, presumably) lines are marked as dirty (ironically, because the code mentioned in bug 330350 comment 1 is not reached in this case). The reflow then starts from the "reused" frames which used to be on the second line, and the first frame (which contains all of the RTL text which is supposed to be split accross two lines) is not reflowed, and therefore not split.

Comment 4

[Boris Zbarsky [:bzbarsky]]

What's marking the frames dirty?

Comment 5

[Uri Bernstein]

(In reply to comment #4)
> What's marking the frames dirty?
> 

I'm afraid I haven't figured out how to answer this. I'm also not absolutely sure that what I wrote above (about the second line being marked dirty) is accurate. All I know is that the first frames actually being reflowed are the in-flows of the second line. The first (primary) frame is not being reflowed.

I'm afraid I'll have to get back to this tomorrow. If you can give me a clue on how to find out what's being marked dirty and by whom, that will help me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

You can set a breakpoint in nsBidiPresUtils::Resolve, then when that's hit set one in nsLineBox::MarkDirty and continue.  See what callers under Resolve() hit MarkDirty.

Comment 7

[Uri Bernstein]

Well, during the second (problematic) reflow (which, BTW, is apparently a resize reflow, not an incremental reflow), there are no calls to MarkDirty() from under nsBidiPresUtils::Resolve(). So Someone else must be doing this.

Comment 8

[Uri Bernstein]

In case it helps here's the relevant portion of the reflow log:

VP 0x239609c r=1 a=21375,10125 c=21375,10125 cnt=1300 
 scroll 0x239627c r=1 a=21375,10125 c=21375,10125 cnt=1301 
  canvas 0x2396130 r=1 a=21375,UC c=21375,UC cnt=1302 
   area 0x2396e00 r=1 a=21375,UC c=21375,UC cnt=1303 
    block 0x2396f84 r=1 incr. (Dirty) a=21375,UC c=21135,UC cnt=1304 
     block 0x2397600 r=0 a=21135,UC c=3975,UC cnt=1305 
      text 0x23976e8 r=0 a=3975,UC c=UC,UC cnt=1306 
      text 0x23976e8 d=3345,270 status=0x1
      text 0x2397a08 r=0 a=3975,UC c=UC,UC pif=0x23976e8 cnt=1307 
      text 0x2397a08 d=2385,270 
      text 0x2396424 r=0 a=1590,UC c=UC,UC cnt=1308 
      text 0x2396424 d=240,270 
      text 0x2397728 r=0 a=1350,UC c=UC,UC cnt=1309 
      text 0x2397728 d=1305,270 status=0x4301
      text 0x2397a7c r=0 a=3975,UC c=UC,UC pif=0x2397728 cnt=1310 
      text 0x2397a7c d=2400,270 
      text 0x239776c r=0 a=1575,UC c=UC,UC cnt=1311 
      text 0x239776c d=75,270 
      img 0x23978b4 r=0 a=1500,UC c=210,150 cnt=1312 
      img 0x23978b4 d=210,150 
     block 0x2397600 d=3975,810 
    block 0x2396f84 d=21135,810 
   area 0x2396e00 d=21375,1050 
  canvas 0x2396130 d=21375,1050 
 scroll 0x239627c d=21375,10125 
VP 0x239609c d=21375,10125 
VP 0x21724f0 r=1 a=21405,11985 c=21405,11985 cnt=1313 
 root 0x2172584 r=1 incr. (Dirty) a=21405,11985 c=21405,11985 cnt=1314 
 root 0x2172584 d=21405,11985 
VP 0x21724f0 d=21405,11985 
Document file:///Users/urib/Documents/HTML/ynet.html loaded successfully
VP 0x21724f0 r=1 a=21405,11985 c=21405,11985 cnt=1315 
 root 0x2172584 r=1 incr. (Dirty) a=21405,11985 c=21405,11985 cnt=1316 
 root 0x2172584 d=21405,11985 
VP 0x21724f0 d=21405,11985 
VP 0x239609c r=1 a=21375,10125 c=21375,10125 cnt=1317 
 scroll 0x239627c r=1 a=21375,10125 c=21375,10125 cnt=1318 
  canvas 0x2396130 r=1 a=21375,UC c=21375,UC cnt=1319 
   area 0x2396e00 r=1 a=21375,UC c=21375,UC cnt=1320 
    block 0x2396f84 r=1 a=21375,UC c=21135,UC cnt=1321 
     block 0x2397600 r=1 incr. (Dirty) a=21135,UC c=3975,UC cnt=1322 
      text 0x2397a08 r=2 a=3975,UC c=UC,UC pif=0x23976e8 cnt=1323 
      text 0x2397a08 d=2385,270 
      text 0x2396424 r=2 a=1590,UC c=UC,UC cnt=1324 
      text 0x2396424 d=240,270 
      text 0x2397728 r=2 a=1350,UC c=UC,UC nif=0x2397a7c cnt=1325 
      text 0x2397728 d=1305,270 status=0x4301
      text 0x2397a7c r=2 a=3975,UC c=UC,UC pif=0x2397728 cnt=1326 
      text 0x2397a7c d=2400,270 
      text 0x239776c r=2 a=1575,UC c=UC,UC cnt=1327 
      text 0x239776c d=75,270 
      img 0x23978b4 r=0 a=1500,UC c=210,150 cnt=1328 
      img 0x23978b4 d=210,150 
     block 0x2397600 d=3975,810 
    block 0x2396f84 d=21135,810 
   area 0x2396e00 d=21375,1050 
  canvas 0x2396130 d=21375,1050 
 scroll 0x239627c d=21375,10125 
VP 0x239609c d=21375,10125 

Notice that the second reflow starts from the second text frame.
Now I'm really off to sleep.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I guess we set the MarkDirty breakpoint and see where it's called from in general, then....

Comment 10

[Uri Bernstein]

(In reply to comment #4)
> What's marking the frames dirty?

It's this code:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.758&mark=5707-5713#5700
(the removed frame is the image frame).

Not that I'm sure how knowing this helps us.
Perhaps the solution is just to mark the lines dirty in EnsureBidiContinuation and RemoveBidiContionuation?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Right.  That's what I was saying in comment 2 -- we should probably twiddle the continuation chain as we do now, then somehow mark those lines dirty...  Problem is how.

Comment 12

[Uri Bernstein]

Attachment: like this?

Pretty ugly, and possibly involving a performance hit, but this is what I managed to come up with.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm not sure whether that's better or whether it's better to hack ReflowDirtyChild to not post a reflow command if called under Resolve() (that is, use a bit in blockframe to control that).  roc, dbaron, what do you think?

Comment 14

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

Can't you pass the blockframe into EnsureBidiContinuation and avoid the loop/QI? Make Resolve() take an actual nsBlockFrame*.

This is pretty much what I was expecting when Boris and I talked on IRC. It won't perform though, it could easily make Resolve be O(N^2). How about keeping a line iterator in Resolve that means "the line containing the current frame"?

Comment 15

[Uri Bernstein]

(In reply to comment #14)
> Can't you pass the blockframe into EnsureBidiContinuation and avoid the
> loop/QI? Make Resolve() take an actual nsBlockFrame*.

I can see how passing the blockframe would avoid the QI, but I think I'll still need the loop, because I need to pass a direct child of the block to FindLineFor().(In reply to comment #14)

> It won't perform though, it could easily make Resolve be O(N^2).

I don't understand why.

> How about keeping
> a line iterator in Resolve that means "the line containing the current frame"?
 
Not sure how to do this, but I can try.

Comment 16

[Uri Bernstein]

(In reply to comment #15)
> (In reply to comment #14)

> > It won't perform though, it could easily make Resolve be O(N^2).
> 
> I don't understand why.

Oh, now I do (I think. Because FindLineFor() is linear, right?)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yeah, exactly.

Comment 18

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

(In reply to comment #15)
> I can see how passing the blockframe would avoid the QI, but I think I'll
> still need the loop, because I need to pass a direct child of the block to
> FindLineFor().

Ah, of course.

Comment 19

[Uri Bernstein]

Attachment: patch

With a line iterator in Resolve().

Comment 20

[Uri Bernstein]

But maybe now I can get rid of the entire aForceReflow thing?

Comment 21

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

(In reply to comment #20)
> But maybe now I can get rid of the entire aForceReflow thing?

Yes, that's the point of the exercise :-)

Other than that, this looks great.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Also, I'd prefer it if we cached end_lines() in a local here.

Comment 23

[Uri Bernstein]

Attachment: patch, no aForceReflow

Also cached end_lines() in a local.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 215104 [details] [diff] [review]
patch, no aForceReflow

>Index: layout/base/nsBidiPresUtils.cpp
>+      if (nsLayoutAtoms::directionalFrame != frameType) {
>+        // Advance the line iterator to the line containing 
frame

So if frameType is directionalFrame we won't advance line... but will still mark it dirty?  Generally this is sorta ok, except if it's still at begin_lines().

It seems to me that we should be doing the advancing at the places where we call MarkDirty(); perhaps with a function that takes the frame, nsBlockFrame::line_iterator& line, const nsBlockFrame::line_iterator& endLines?  That would also address the second concern I had, which is that the code as it stands always iterates the lines, even if it doesn't actually change any continuations.

Other than that, this looks most excellent.

Comment 25

[Uri Bernstein]

(In reply to comment #24)
> (From update of attachment 215104 [details] [diff] [review] [edit])
> >Index: layout/base/nsBidiPresUtils.cpp
> >+      if (nsLayoutAtoms::directionalFrame != frameType) {
> >+        // Advance the line iterator to the line containing 
> frame
> 
> So if frameType is directionalFrame we won't advance line... but will still
> mark it dirty?

No, because both calls to markDirty are inside the "else" block of |if (nsLayoutAtoms::directionalFrame == frameType)|.

> 
> It seems to me that we should be doing the advancing at the places where we
> call MarkDirty(); perhaps with a function that takes the frame,
> nsBlockFrame::line_iterator& line, const nsBlockFrame::line_iterator& endLines?

But then we will be calling this function with continuation frames that we just created (in the previous iteration). I'm not sure these frames even know what line they're on. (notice that we are not taking |frame| from mLogicalFrames with each iteration of the loop).

Comment 26

[Boris Zbarsky [:bzbarsky]]

> No, because both calls to markDirty are inside the "else" block 

Ah, ok.  Worth asserting that at the callsite so it doesn't change.

> notice that we are not taking |frame| from mLogicalFrames with each iteration
> of the loop

We could use another var that's set whenever we get frame from mLogicalFrames...

But either way is ok, I guess.  Let's do this and see what the profiles look like.

Comment 27

[Uri Bernstein]

(In reply to comment #26)
> > No, because both calls to markDirty are inside the "else" block 
> 
> Ah, ok.  Worth asserting that at the callsite so it doesn't change.
> 

I know you usually win these arguments, but I'll try anyhow:

I don't see the point of asserting that the frame is not a directionalFrame at that point. If it is a directionalFrame (and, in fact, if it is anything other than a textFrame, which should be assured by the |if (isTextFrame)| we're inside) than nothing we're doing makes any sense (creating continuations for directional frames??), and marking the wrong line (or an invalid line) dirty is the least of our concerns. But then again, I think it's clear enough, just from looking at the code, that this cannot happen.

> We could use another var that's set whenever we get frame from
> mLogicalFrames...
> 
> But either way is ok, I guess.  Let's do this and see what the profiles look
> like.
> 

So, with your permission, I'll leave it this way (which I find to be more elegant) for now, and as you suggest, if profiles show this is significant I'll do what you suggest.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> than nothing we're doing makes any sense 

OK, that I buy.  ;)  No need for the assert; let's land this as-is if roc likes it.

> think it's clear enough, just from looking at the code

I don't buy that, though, for future reference.  I've seen too many cases when code gets changed without looking at how that change affects other code 20 lines away, so I like to assert assumptions right where they're made...

Comment 29

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

Comment on attachment 215104 [details] [diff] [review]
patch, no aForceReflow

I think the optimization bz suggested is worthwhile, but I'm happy to have it happen in a separate patch.

Comment 30

[Uri Bernstein]

Checked in:
Checking in layout/base/nsBidiPresUtils.h;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.h,v  <--  nsBidiPresUtils.h
new revision: 1.21; previous revision: 1.20
done
Checking in layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.73; previous revision: 1.72
done
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.762; previous revision: 3.761
done

Comment 31

[Uri Bernstein]

For me, with the testcase in bug 319930, this only improved performance  marginally, if at all. I suspect that for that case, all lines end up reflowed anyway.