Closed Bug 635329 Opened 10 years ago Closed 10 years ago

Crash [@ nsBidiPresUtils::RemoveBidiContinuation] with large margin, changing text node

Categories

(Core :: Layout: Text and Fonts, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: smontagu)

References

(Blocks 3 open bugs)

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:dos (critical w/out frame-poisoning)] [softblocker][has patch])

Crash Data

Attachments

(3 files, 1 obsolete file)

No description provided.
Nominating for blocking because this might be the same as a topcrash with the same signature (bug 632828).
blocking2.0: --- → ?
Attached file stack trace (mac)
OS: Windows 7 → All
Assignee: nobody → smontagu
blocking2.0: ? → final+
Whiteboard: [sg:dos (critical w/out frame-poisoning)] → [sg:dos (critical w/out frame-poisoning)] [softblocker]
This turns out to be caused by bug 597627 deleting frames during bidi resolution. I think that we won't be losing much if we just don't do that optimization when SetLength is called from AdjustOffsetsForBidi, since bidi resolution blows away text runs for all frames it touches anyway. Mats, does that make sense?
Depends on: 597627
Attached patch Patch (obsolete) — Splinter Review
Attachment #513850 - Flags: review?(roc)
Attachment #513850 - Flags: feedback?(matspal)
Whiteboard: [sg:dos (critical w/out frame-poisoning)] [softblocker] → [sg:dos (critical w/out frame-poisoning)] [softblocker][has patch]
Blocks: 597627
No longer depends on: 597627
Keywords: regression
Comment on attachment 513850 [details] [diff] [review]
Patch

Yes, this looks good to me.
Nit: prefix the parameter with "a"
Attachment #513850 - Flags: feedback?(matspal) → feedback+
Comment on attachment 513850 [details] [diff] [review]
Patch

+                 PRBool inBidiResolution = PR_FALSE);

I think you should just have a flags word with a flag "ALLOW_FRAME_CREATION_DESTRUCTION", and the default (0) should not allow it.
Attachment #513850 - Attachment is obsolete: true
Attachment #513896 - Flags: review?(roc)
Attachment #513850 - Flags: review?(roc)
Comment on attachment 513896 [details] [diff] [review]
Addressed comments

+#define ALLOW_FRAME_CREATION_DESTRUCTION 1

Hmm, might as well make it ALLOW_FRAME_CREATION_AND_DESTRUCTION :-). Also, make it 0x01. Also, should be an enum in nsTextFrame.h next to SetLength, i.e.
  enum { ALLOW_FRAME_CREATION_AND_DESTRUCTION = 0x01 };
Attachment #513896 - Flags: review?(roc) → review+
Comment on attachment 513896 [details] [diff] [review]
Addressed comments

This should be very safe since it just enables selective non-use of the optimizations added in bug 597627.
Attachment #513896 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/3db6693e136e
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 633476
Duplicate of this bug: 632828
Crash Signature: [@ nsBidiPresUtils::RemoveBidiContinuation]
You need to log in before you can comment on or make changes to this bug.