[regression] Duplicated text in certain bidi case with image

RESOLVED FIXED in mozilla1.9alpha1

Status

()

--
major
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: uriber, Assigned: uriber)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla1.9alpha1
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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).
(Assignee)

Comment 1

13 years ago
Created attachment 214938 [details]
testcase

Description is in comment #0.
> 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?
(Assignee)

Comment 3

13 years ago
(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.
What's marking the frames dirty?
(Assignee)

Comment 5

13 years ago
(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.
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.
(Assignee)

Comment 7

13 years ago
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.
(Assignee)

Comment 8

13 years ago
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.
I guess we set the MarkDirty breakpoint and see where it's called from in general, then....
(Assignee)

Comment 10

13 years ago
(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?
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.
(Assignee)

Comment 12

13 years ago
Created attachment 215042 [details] [diff] [review]
like this?

Pretty ugly, and possibly involving a performance hit, but this is what I managed to come up with.
Attachment #215042 - Flags: review?(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?
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"?
(Assignee)

Comment 15

13 years ago
(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.
(Assignee)

Comment 16

13 years ago
(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?)
Yeah, exactly.
(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.
(Assignee)

Comment 19

13 years ago
Created attachment 215064 [details] [diff] [review]
patch

With a line iterator in Resolve().
Attachment #215042 - Attachment is obsolete: true
Attachment #215064 - Flags: superreview?(bzbarsky)
Attachment #215064 - Flags: review?(roc)
Attachment #215042 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

13 years ago
But maybe now I can get rid of the entire aForceReflow thing?
(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.
Also, I'd prefer it if we cached end_lines() in a local here.
Blocks: 330350
(Assignee)

Comment 23

13 years ago
Created attachment 215104 [details] [diff] [review]
patch, no aForceReflow

Also cached end_lines() in a local.
Attachment #215064 - Attachment is obsolete: true
Attachment #215104 - Flags: superreview?(bzbarsky)
Attachment #215104 - Flags: review?(roc)
Attachment #215064 - Flags: superreview?(bzbarsky)
Attachment #215064 - Flags: review?(roc)
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.
Attachment #215104 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 25

13 years ago
(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).
> 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.
(Assignee)

Comment 27

13 years ago
(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.
> 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 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.
Attachment #215104 - Flags: review?(roc) → review+
(Assignee)

Comment 30

13 years ago
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
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 31

13 years ago
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.
(Assignee)

Updated

13 years ago
Depends on: 339935

Updated

12 years ago
Attachment #214938 - Attachment mime type: text/html → text/html; charset=windows-1255
(Assignee)

Updated

11 years ago
Depends on: 423676

Updated

10 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.