Closed Bug 385270 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: roc)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 4 obsolete files)

Loading the testcase triggers:

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /Users/jruderman/trunk/mozilla/gfx/thebes/src/gfxSkipChars.cpp, line 92
Attached patch clean up text frame offsets (obsolete) — Splinter Review
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.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #269619 - Flags: review?(smontagu)
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #269619 - Attachment is obsolete: true
Attachment #269627 - Flags: review?(smontagu)
Attachment #269619 - Flags: review?(smontagu)
This patch passes reftests, BTW.
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
Okay, I'll make it shrinkable as well
Attached patch fix (obsolete) — Splinter Review
OK, this makes AdjustOffsetsForBidi more robust.
Attachment #269627 - Attachment is obsolete: true
Attachment #269754 - Flags: review?(smontagu)
Attachment #269627 - Flags: review?(smontagu)
Hmm, that assertion I added in AdjustOffsetsForBidi is firing. Simon, can you tell me exactly under what circumstances AdjustOffsetsForBidi is called?
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.
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?
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.
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.
Attachment #270535 - Flags: superreview?(dbaron)
Attachment #270535 - Flags: review?(dbaron)
Blocks: 385526
Attachment #270535 - Flags: superreview?(dbaron)
Attachment #270535 - Flags: superreview+
Attachment #270535 - Flags: review?(dbaron)
Attachment #270535 - Flags: review+
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.
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.
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()".
Also Nickolay, you had both these patches in, not just the first one, right? Because we definitely need both.
Oops, sorry. We need a testcase or debug information or something.
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.
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).
I guess we should mark this FIXED. I'll file a new bug on Nickolay's regression.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #269754 - Flags: review?(smontagu) → review+
Was there a new bug filed on Nickolay's regression mentioned in comment 18? If so, which bug number?
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.
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.

The fix for this bug was backed out to fix some regressions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The backout has caused bug 386584 to return.
Blocks: 391127
Blocks: 391092
Attached patch better fixSplinter Review
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.
Attachment #269754 - Attachment is obsolete: true
Attachment #270535 - Attachment is obsolete: true
Attachment #275710 - Flags: review?(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.
Attachment #275710 - Flags: review?(smontagu) → review+
Comment on attachment 275710 [details] [diff] [review]
better fix

second attempt at fixing the text frame offset issue
Attachment #275710 - Flags: approval1.9?
Blocks: 391758
Comment on attachment 275710 [details] [diff] [review]
better fix

a1.9=dbaron
Attachment #275710 - Flags: approval1.9? → approval1.9+
Checked in.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Depends on: 392435
Blocks: 390417
Blocks: 391205
Flags: in-testsuite?
The first testcase in this bug is an existing reftest.
Flags: in-testsuite? → in-testsuite+
Depends on: 409375
Blocks: 351650
Depends on: 419285
You need to log in before you can comment on or make changes to this bug.