Closed Bug 538062 Opened 12 years ago Closed 12 years ago

"ASSERTION: negative length" and more with bidi in <option>

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jruderman, Assigned: roc)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical][critsmash:resolved])

Attachments

(4 files, 1 obsolete file)

Attached file testcase
This testcase triggers some textframe assertions.  In asciibetical order:

###!!! ASSERTION: Attempting to allocate excessively large array: 'Error', file nsTArray.cpp, line 69
###!!! ASSERTION: Bad offset: 'aPos <= aFrag->GetLength()', file layout/generic/nsTextFrameThebes.cpp, line 512
###!!! ASSERTION: Content offset/length out of bounds: 'offset + limitLength == PRInt32(frag->GetLength())', file layout/generic/nsTextFrameThebes.cpp, line 6242
###!!! ASSERTION: Creating ContinuingTextFrame, but there is no more content: 'mContentOffset < PRInt32(aContent->GetText()->GetLength())', file layout/generic/nsTextFrameThebes.cpp, line 3508
###!!! ASSERTION: No text for IsSpace!: 'aLength > 0', file layout/generic/nsTextFrameThebes.cpp, line 540
###!!! ASSERTION: No text for IsSpace!: 'aPos < aFrag->GetLength()', file layout/generic/nsTextFrameThebes.cpp, line 558
###!!! ASSERTION: Should have been cleared: 'mBreakSinks.IsEmpty()', file layout/generic/nsTextFrameThebes.cpp, line 649
###!!! ASSERTION: Should have been cleared: 'mLineBreakBeforeFrames.IsEmpty()', file layout/generic/nsTextFrameThebes.cpp, line 651
###!!! ASSERTION: Should have been cleared: 'mMappedFlows.IsEmpty()', file layout/generic/nsTextFrameThebes.cpp, line 652
###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file nsTextFragment.h, line 186
###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file nsTextFrame.h, line 319
I have a patch for this inside Bidi resolution that "works", i.e. prevents the assertions and leaves the frame tree in a sane state, but I think the root cause of the bug is happening somewhere else when the content changes, before Bidi resolution takes place.

After bidi resolution happens on the original text, after the appendChild() but before the removeChild(), the frame tree looks like this:

Block(select)(0)@0xaeb05198 next=0xaeb05530 [content=0xb361f240] {180,180,1740,1020} [state=00000400] sc=0xaeb03fb8(i=1,b=0) pst=:-moz-display-comboboxcontrol-frame<
                    line 0xaeb05250: count=2 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x10141] {240,60,480,900} <
                      Text(-1)@0xaeb05208[0,2,F]  next=0xb14c81c8 next-continuation=0xb14c81c8 {240,60,480,900} [state=90620000] [content=0xb2842c70] sc=0xaeb05140 pst=:-moz-non-element<
                        "B "
                      >
                      Text(-1)@0xb14c81c8[2,1,T]  prev-continuation=0xaeb05208 {0,0,0,0} [state=00020402] [content=0xb2842c70] sc=0xaeb05140 pst=:-moz-non-element<
                        "\u064a"
                      >
                    >
                  >

Then after the removeChild, before bidi re-resolution, it looks like this:
                  Block(select)(0)@0xaeb05198 next=0xaeb05530 [content=0xb361f240] {180,180,1500,1020} [state=00000400] sc=0xaeb03fb8(i=1,b=0) pst=:-moz-display-comboboxcontrol-frame<
                    line 0xaeb05250: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x10100] {240,60,1260,900} <
                      Text(-1)@0xaeb05208[0,2,F]  next=0xb14c81c8 next-continuation=0xb14c81c8 {240,60,720,900} [state=90220000] [content=0xb2842c70] sc=0xaeb05140 pst=:-moz-non-element<
                        "\u064a\u5a5a"
                      >
                      Text(-1)@0xb14c81c8[2,-1,T]  prev-continuation=0xaeb05208 {960,60,540,900} [state=80420000] [content=0xb2842c70] sc=0xaeb05140 pst=:-moz-non-element<
                        ""
                      >
                    >
                  >

In other words, the content has changed, but the offsets in the frames still reflect the old content.

The interesting thing is that the same content appears twice in the frame tree, once as copied here, directly below the select, and again further down in a "selectPopupList". When bidi resolution happens on the block in the selectPopupList after the removeChild, the content seems to have been reframed somehow: 
                        Block(option)(0)@0xaeb03d98 [content=0xb37f1600] {0,0,2940,900} [state=00005000] sc=0xaeb03be0(i=1,b=0)<
                          line 0xaeb03ef0: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8141] {180,0,1260,900} <
                            Text(0)@0xb14c8180[0,1,T]  {900,0,540,900} [state=c0424000] [content=0xb3625a60] sc=0xaeb03e00 pst=:-moz-non-element<
                              "\u064a"

So I think the real fix would be to make the same reframing happen on both copies of the content, but I don't know where it happens.
Attached patch wallpaper patchSplinter Review
Assignee: nobody → smontagu
Whiteboard: [sg:critical?]
The problem is probably that nsComboboxControlFrame::ActuallyDisplayText doesn't notify on the primary text frame. It should.
Simon, will you be able to work on this soon, to do the fix in comment #3? Or shall I assign it to someone else?
I did try to work on this, but either I didn't understand comment 3, or it didn't help
Assignee: smontagu → roc
blocking2.0: --- → ?
Attached patch fixSplinter Review
Turns out the underlying issue is that we forgot to set the primary frame for the mDisplayContent. So when the mDisplayContent text was changed, we never calls CharacterDataChanged on the frame.
Attachment #437984 - Flags: review?(matspal)
Attached patch Remove mTextFrame field (obsolete) — Splinter Review
Followup patch. Holding a reference to mTextFrame is dangerous and bogus if there are actually many text frames representing the node. Anyway it turns out mTextFrame is not actually needed, we can just use a local variable instead in CreateFrameFor.
Attachment #437986 - Flags: review?(matspal)
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
The two patches are the same.
I hate that. Give me some HTML5 File API love!
Attachment #437986 - Attachment is obsolete: true
Attachment #437986 - Flags: review?(matspal)
mats, can you please review this?
It looks like nsCSSFrameConstructor::FindPrimaryFrameFor used to do this
for us "on demand", so I'm guessing this bug is a regression from removing
the primary frame map (and thus does not affect 1.9.2 or older)
Depends on: 500882
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 500882
No longer depends on: 500882
Comment on attachment 437984 [details] [diff] [review]
fix

Looks good but you need to fix
nsGfxButtonControlFrame::CreateFrameFor in the same way.
Also, nsCSSFrameConstructor::CreateAnonymousFrames should
probably assert that content->GetPrimaryFrame() is non-null
on line 3887, i.e. when CreateFrameFor() returns a non-null
frame.  I don't think they need to be the same though.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#3859

r=mats with that addressed
Attachment #437984 - Flags: review?(matspal) → review+
Whiteboard: [sg:critical?][needs review] → [sg:critical?]
Attachment #437994 - Flags: review+
Thanks, sounds good. Yes, they don't need to be the same.
Whiteboard: [sg:critical?] → [sg:critical?][needs landing]
Attachment #437984 - Flags: superreview?(bzbarsky)
Whiteboard: [sg:critical?][needs landing] → [sg:critical?][needs superreview]
Can we get a call as to whether or not this as an sg:crit?
I think it should be, since all kinds of weirdness can happen. No point in agonizing over it since we have a patch.
Whiteboard: [sg:critical?][needs superreview] → [sg:critical][needs superreview]
Whiteboard: [sg:critical][needs superreview] → [sg:critical][needs superreview][critsmash:patch]
Attachment #437984 - Flags: superreview?(bzbarsky) → superreview?(dbaron)
Comment on attachment 437984 [details] [diff] [review]
fix

sr=dbaron
Attachment #437984 - Flags: superreview?(dbaron) → superreview+
Whiteboard: [sg:critical][needs superreview][critsmash:patch] → [sg:critical][needs landing][critsmash:patch]
I checked this in:
http://hg.mozilla.org/mozilla-central/rev/489f29b06ef5
http://hg.mozilla.org/mozilla-central/rev/4af9e6180b76
I backed out the nsGfxButtonControlFrame change because it broke accessibility tests:
http://hg.mozilla.org/mozilla-central/rev/a908be341373
and filed bug 565569 on fixing nsGfxButtonControlFrame.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical][needs landing][critsmash:patch] → [sg:critical][critsmash:patch]
Flags: in-testsuite?
Whiteboard: [sg:critical][critsmash:patch] → [sg:critical][critsmash:resolved]
blocking2.0: ? → final+
Priority: -- → P2
Group: core-security
Crashtest: http://hg.mozilla.org/mozilla-central/rev/ef75056ce708
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.