Last Comment Bug 534768 - Crash [@ nsBidiPresUtils::Resolve]
: Crash [@ nsBidiPresUtils::Resolve]
Status: RESOLVED FIXED
[sg:critical?]
: assertion, crash, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
-- critical (vote)
: mozilla1.9.3a4
Assigned To: Timothy Nikkel (:tnikkel)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: randomstyles
  Show dependency treegraph
 
Reported: 2009-12-14 16:51 PST by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
9 users (show)
tnikkel: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
needed
.4-fixed
needed
.10-fixed


Attachments
testcase 1 (aborts) (366 bytes, text/html)
2009-12-14 16:52 PST, Jesse Ruderman
no flags Details
testcase 2 (crashes) (337 bytes, text/html)
2009-12-14 16:53 PST, Jesse Ruderman
no flags Details
patch (9.43 KB, patch)
2009-12-15 02:51 PST, Timothy Nikkel (:tnikkel)
roc: review+
smontagu: review+
Details | Diff | Splinter Review
1.9.2 patch (10.74 KB, patch)
2010-03-23 14:48 PDT, Timothy Nikkel (:tnikkel)
roc: review+
dveditz: approval1.9.2.4+
Details | Diff | Splinter Review
1.9.1 patch (11.29 KB, patch)
2010-03-23 14:48 PDT, Timothy Nikkel (:tnikkel)
roc: review+
dveditz: approval1.9.1.10+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2009-12-14 16:51:15 PST
Variations on bug 429865's crashtest still cause problems.
Comment 1 User image Jesse Ruderman 2009-12-14 16:52:10 PST
Created attachment 417591 [details]
testcase 1 (aborts)

###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file layout/generic/nsBlockFrame.cpp, line 4764

###!!! ASSERTION: aFrame is not a descendent of aBlockFrame: 'parent', file layout/base/nsBidiPresUtils.cpp, line 285

###!!! ASSERTION: Can't find frame in lines!: 'hasNext', file layout/base/nsBidiPresUtils.cpp, line 291

###!!! ABORT: comparing iterators over different lists: 'mListLink == aOther.mListLink', file layout/base/../generic/nsLineBox.h, line 704
Comment 2 User image Jesse Ruderman 2009-12-14 16:53:00 PST
Created attachment 417592 [details]
testcase 2 (crashes)

###!!! ASSERTION: Should have been cleared: 'mBreakSinks.IsEmpty()', file layout/generic/nsTextFrameThebes.cpp, line 649

###!!! ASSERTION: invalid array index: 'i < Length()', file ../../dist/include/nsTArray.h, line 317

Crash [@ nsBidiPresUtils::Resolve] accessing 0x55555555 (with MallocScribble)
Comment 3 User image Timothy Nikkel (:tnikkel) 2009-12-14 19:51:59 PST
Looking at the first testcase, stripping out all the dynamic stuff, a text frame continuation stays on the firstletter frame's overflow list because of the code here http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFirstLetterFrame.cpp#205 where we invent a linelayout from nothing, and SetDirtyNextLine gets called on that linelayout, nothing ever comes of the SetDirtyNextLine call because we forget about that linelayout right away.

As a side note, can we assert that nothing is on overflow lists after a flush and/or when entering frame construction (anywhere else?)? I've seen a few Jesse bugs that would have probably been found much quicker if we asserted such.
Comment 4 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-14 20:29:50 PST
When we invent a linelayout from nothing, we have to not wrap anything on the line.

Yes, we could assert that nothing is on overflow lists, apart from the above issue which I'm not sure how to fix. But in the past we've tried to survive having content left on overflow lists. Probably we should try to survive it still.
Comment 5 User image Timothy Nikkel (:tnikkel) 2009-12-15 02:48:40 PST
The SetDirtyNextLine was just a symptom of the real problem, we weren't trying to wrap to another line, the first letter logic was causing the continuation to be created.

I think what we need is to do the same thing as bug 491547 but in nsFirstLetterFrame::Reflow in the case where we create a next-in-flow for a child of a floating letter frame. Normally we'd create a continuation for the textframe child of a floating first letter frame if it might need one ahead of time in nsCSSFrameConstructor::CreateFloatingLetterFrame. But that doesn't correctly detect that this situation needs a continuation, and I don't think in general that it could easily be adapted to be able to handle other dynamic situations.
Comment 6 User image Timothy Nikkel (:tnikkel) 2009-12-15 02:51:46 PST
Created attachment 417662 [details] [diff] [review]
patch

Hopefully this situation only happens on BIDI frames so that we can use nextBidi to insert the continuation where we need to without triggering a reflow command.
Comment 7 User image Timothy Nikkel (:tnikkel) 2010-02-09 20:36:37 PST
Simon, do you want to review this or are you fine with just roc reviewing?
Comment 8 User image Simon Montagu :smontagu 2010-02-10 10:47:29 PST
Comment on attachment 417662 [details] [diff] [review]
patch

Apologies for the delay
Comment 9 User image Timothy Nikkel (:tnikkel) 2010-02-12 23:53:14 PST
I remembered that I wanted to look into this more. Turns out you don't need any BIDI to trigger the condition of needing to create a continuation for a textframe inside a floating first-letter frame. A block with floating first-letter style that contains only a space followed by a character that is not punctuation, not a space, and not alphanumeric will do it. So the "should only get here with bidi frames" assertion will fire.

We need to use the nextBidi listname when inserting the continuation so that we don't try to issue a another reflow from inside a reflow. So I propose removing the assertion and making it clear in a comment that the listname nextBidi is bogus but necessary.
Comment 10 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-16 14:41:28 PDT
Sounds good to me. Please do that and let's get this fixed!
Comment 11 User image Timothy Nikkel (:tnikkel) 2010-03-17 16:37:22 PDT
Made those changes and landed
http://hg.mozilla.org/mozilla-central/rev/aff1d6e46f2c
Comment 12 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-03-19 13:20:42 PDT
Can we get a crashtest, as well as branch patches worked up?
Comment 13 User image Timothy Nikkel (:tnikkel) 2010-03-23 14:45:06 PDT
What do you mean by "get a crashtest"? For security bugs the procedure I've been using is to set "in-testsuite?", and then when the bug is opened the testcase can be landed. Is there a better way I should be handling it?
Comment 14 User image Timothy Nikkel (:tnikkel) 2010-03-23 14:48:12 PDT
Created attachment 434362 [details] [diff] [review]
1.9.2 patch

Enough changes that I liked a second set of eyes on this.

The extra queryframe stuff is needed because nsIFrame and nsIFrameDebug (gone on m-c) make it ambiguous otherwise.
Comment 15 User image Timothy Nikkel (:tnikkel) 2010-03-23 14:48:47 PDT
Created attachment 434363 [details] [diff] [review]
1.9.1 patch
Comment 16 User image Daniel Veditz [:dveditz] 2010-04-02 13:55:45 PDT
(In reply to comment #13)
> What do you mean by "get a crashtest"? For security bugs the procedure I've
> been using is to set "in-testsuite?", and then when the bug is opened the
> testcase can be landed. Is there a better way I should be handling it?

Not landing the test and using "in-testsuite?" is great, that's what we want for security bugs. However, we do like to have the test in hand, both so your reviewer can review it (does it have sufficient coverage,  etc) and so QA can manually apply it to their trees and test with it.
Comment 17 User image Daniel Veditz [:dveditz] 2010-04-02 13:56:41 PDT
Comment on attachment 434362 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.4, a=dveditz for release-drivers
Comment 18 User image Daniel Veditz [:dveditz] 2010-04-02 13:56:50 PDT
Comment on attachment 434363 [details] [diff] [review]
1.9.1 patch

Approved for 1.9.1.10, a=dveditz for release-drivers
Comment 19 User image Timothy Nikkel (:tnikkel) 2010-04-02 14:14:25 PDT
In this case the test is just the two testcases in this bug imported into the tree as crashtests. So the reviewer can see them and QA can load them directly without needing to apply anything.
Comment 20 User image Timothy Nikkel (:tnikkel) 2010-04-06 17:10:53 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a1386f73f6ec
Comment 21 User image Timothy Nikkel (:tnikkel) 2010-04-06 17:13:16 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9fa95b396a34
Comment 22 User image Al Billings [:abillings] 2010-04-07 15:17:12 PDT
Verified for 1.9.2 using the two attached tests. Aborts and crashes (respectively) on 1.9.2.3 but has no problems with my build from today (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4pre) Gecko/20100407 Lorentz/3.6.4pre (.NET CLR 3.5.30729)).

Verified for 1.9.1 the same way with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10pre) Gecko/20100407 Shiretoko/3.5.10pre (.NET CLR 3.5.30729).
Comment 23 User image Timothy Nikkel (:tnikkel) 2010-06-27 17:24:02 PDT
Added crashtest
http://hg.mozilla.org/mozilla-central/rev/dbd20f123483

Note You need to log in before you can comment on or make changes to this bug.