385270 - "ASSERTION: Invalid offset" in gfxSkipCharsIterator::SetOffsets with :first-letter

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase (reftests/first-letter/basic-1.html)

Loading the testcase triggers:

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /Users/jruderman/trunk/mozilla/gfx/thebes/src/gfxSkipChars.cpp, line 92

Comment 1

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

Attachment: clean up text frame offsets

This bug is because I'm incorrectly handling textrun construction when frame content offsets are overlapping. Rather than write yet more code to handle that case, I think it's time we fixed text frames so that their content offsets never overlap. This patch does that. However, it does depend on nsBidiPresUtils cooperating and making sure that those ranges do not overlap.

Comment 2

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

Attachment: updated patch

This is an improved patch. It reverses some of the fix to bug 385344 because now that all text frame offsets are always accurate, the reflow of first-line and first-letter text can take care of invalidating the textrun for their next-in-flows if necessary. (We couldn't do that before because we had to worry about the possibility of the next-in-flow's textrun being recreated before the next in flow had a chance to update its own text offsets.)

It also creates GetContentEnd() and uses it in a bunch of places.

Also I noticed that nsBidiPresUtils relies on AdjustOffsetsForBidi consuming all the text for the in-flow-chain. That means we have to call AdjustNextInFlowContentOffsetsForGrowth from there.

Comment 3

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

This patch passes reftests, BTW.

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 269627 [details] [diff] [review]
updated patch

I'm not sure if the change to AdjustOffsetsForBidi is right for both of the call sites from nsBidiPresUtils. Text runs can both grow and shrink during Bidi resolution, especially during text editing.

Apart from that I found two typos in comments: pram for param at https://bugzilla.mozilla.org/attachment.cgi?id=269627&action=diff#mozilla/layout/generic/nsTextFrameThebes.cpp_sec2 and exmaple for example at https://bugzilla.mozilla.org/attachment.cgi?id=269627&action=diff#mozilla/layout/generic/nsTextFrameThebes.cpp_sec11

Comment 5

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

Okay, I'll make it shrinkable as well

Comment 6

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

Attachment: fix

OK, this makes AdjustOffsetsForBidi more robust.

Comment 7

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

Hmm, that assertion I added in AdjustOffsetsForBidi is firing. Simon, can you tell me exactly under what circumstances AdjustOffsetsForBidi is called?

Comment 8

[Simon Montagu :smontagu]

I hope Uri has time to look at this too. I think AdjustOffsetsForBidi can be called on a non-first-in-flow if line breaking has already happened. Bug 382422 will make that much less common, but it could still happen during incremental reflow of a large block.

Comment 9

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

Ok ... in that case, is it OK to dispose of the next-in-flows the way I'm doing it here, setting them all to empty?

Comment 10

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

We discussed this on IRC. We concluded the current patch should be safe, EXCEPT for the calls to AdjustOffsetsForBidi from nsLineLayout. However those calls should really not be needed.

Comment 11

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

Attachment: testing the theory that this code is unnecessary

Here's the argument why this code is not necessary:

-- the first call to AdjustOffsetsForBidi can only change the end-offset of the text frame, putting it back to what it was before the text frame was reflowed. The reflow change, and the change back, cannot be changing a non-fluid continuation boundary. We're going to reflow this frame again, and that reflow will not be affected by the current end-offset (it could be affected by a non-fluid continuation boundary, but that hasn't changed), but that reflow will reset the frame's end-offset to some new value. So in the end we will converge to the same state as if we'd never done this AdjustOffsetsForBidi.

-- in the second call to AdjustOffsetForBidi, we're changing the start offset of the next-in-flow. This is not needed because as soon as the next-in-flow is reflowed, it will hit
mContentOffset = prevInFlow->GetContentOffset() + prevInFlow->GetContentLength();
in nsTextFrame::Reflow, which does the same thing.

Comment 12

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

thanks, checked that in. Let's wait for a couple of days and then push ahead with the real patch if there are no problems.

Comment 13

[Nickolay_Ponomarev]

FWIW, it looks like attachment 270535 [details] [diff] [review] causes a hang on startup (with a session of > 40 tabs). If I am to believe a debugger with an opt build, the hang happens in nsBlockFrame::ReflowInlineFrames, not sure if it's useful information.

I backed out the patch locally and the hang went away. After I re-applied it, Firefox started hanging again. I'm not sure I'll be able to provide steps to reproduce, but I'm trying to figure out if a specific page causes this.

Comment 14

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

Nickolay: if you can get the hang in a debug build, it would be great to see a frame tree. Just break in the debugger and then if you're in a frame method, run "this->DumpFrameTree()".

Comment 15

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

Also Nickolay, you had both these patches in, not just the first one, right? Because we definitely need both.

Comment 16

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

Oops, sorry. We need a testcase or debug information or something.

Comment 17

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

Oh dear. It looks like I checked in attachment 269754 [details] [diff] [review] by mistake three weeks ago, when I landed the textrun word cache.

Simon pretty much reviewed it already, though.

Comment 18

[Nickolay_Ponomarev]

For the reference, I told roc on IRC that loading http://en.wikipedia.org/wiki/Grindhouse_%28film%29 and Ctrl++'ing it three times quickly triggers my hang (or NS_ABORT in a debug build).

Comment 19

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

I guess we should mark this FIXED. I'll file a new bug on Nickolay's regression.

Comment 20

[Martijn Wargers (dead)]

Was there a new bug filed on Nickolay's regression mentioned in comment 18? If so, which bug number?

Comment 21

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

bug 389603

Comment 22

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

I think we're going to have to back this out for M7. The bidi resolver really messes with the text frame offsets, and it's going to be difficult to fix that. Also, there are situations where a frame partially fits so we return NOT_COMPLETE and adjust the content length in the frame to be the amount that fitted; but then instead of creating a new continuation right away, we end up pushing the frame; eventually we reflow it again, but until then we're in a state where not all the content is mapped by text frames. This is bad. I need to think about this more.

Comment 23

[Caio Tiago Oliveira (asrail)]

This:

#0  nsExpirationTracker<gfxFont, 3u>::AddObject (this=0x0, aObj=0x18df740)
    at ../../../dist/include/xpcom/nsExpirationTracker.h:125
#1  0x00002aaaac2753eb in gfxFontCache::NotifyReleased (this=0x18df760, aFont=0x18df740)
    at /home/asrail/mozbuilds/mozilla/gfx/thebes/src/gfxFont.cpp:133

on closing might to be relatd to this?

I've got it after loading http://blog.vlad1.com/ and closing... but I couldn't make it to crash again, after several tries.

Comment 24

[Jesse Ruderman]

The fix for this bug was backed out to fix some regressions.

Comment 25

[Steve England [:stevee]]

The backout has caused bug 386584 to return.

Comment 27

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

Attachment: better fix

Okay, this is the new approach I talked about in my newsgroup post; it's also described in comments here. Only mContentOffset is now treated as authoritative. Also I take extreme defensive measures in AdjustOffsetsForBidi to deflect bidi resolver bugs. This fixes this bug and shacknews.com and (in conjunction with the patch in bug 390050) various columns-plus-text-resizing assertions and crashes that have been observed at blog.vlad1.com, weblogs.mozillazine.org/roc, and en.wikipedia.org.

Comment 28

[Simon Montagu :smontagu]

Comment on attachment 275710 [details] [diff] [review]
better fix

> nsTextFrame::Init(nsIContent*      aContent,

>   // Note that if we're created due to bidi splitting the bidi code
>   // will override what we compute here, so it's ok.
>   // We're not a continuing frame.
>   // mContentOffset = 0; not necessary since we get zeroed out at init
>-  mContentLength = GetInFlowContentLength();
>-  return rv;
>+  return nsFrame::Init(aContent, aParent, aPrevInFlow);

I think the comment about the bidi code overriding what we compute here refers to the line you're removing, so it can go to.

Other than that, looks good.

Comment 29

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

Comment on attachment 275710 [details] [diff] [review]
better fix

second attempt at fixing the text frame offset issue

Comment 30

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

Comment on attachment 275710 [details] [diff] [review]
better fix

a1.9=dbaron

Comment 31

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

Checked in.

Comment 33

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

The first testcase in this bug is an existing reftest.