887631 - Crash with bidi, selection.modify

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires pref) (requires focus)

With:
  user_pref("bidi.browser.ui", true);

###!!! ASSERTION: aNode isn't in mRange, or something else weird happened: 'NS_SUCCEEDED(res) && !nodeBefore && !nodeAfter', file content/base/src/nsContentIterator.cpp, line 1430

###!!! ASSERTION: bad index: 'uint32_t(aIndex) < mState.mLength', file nsTextFragment.h, line 162

Crash HasTerminalNewline | nsTextFragment::CharAt

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Simon Montagu :smontagu]

This is rather annoying to investigate, since I find that it only crashes once per profile and I need to create a new profile for every test run.

FWIW there is a regression range of http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1fb34b07c17&tochange=9db46ddfb517, but it looks as if that just means that bug 765780 exposed an existing bug.

Comment 3

[Simon Montagu :smontagu]

From the stack it looks as if the problem is that the call to nsCaret::InvalidateOutsideCaret from PresShell::CharacterDataChanged is inspecting frame content at a point where the content has changed but the pointers to content cached on the frame haven't been updated yet.

Comment 4

[Simon Montagu :smontagu]

On the other hand, that only refers to the crash with bidi.browser.ui=true; the first assertion occurs even without that pref set, so the "something weird" which is happening needs independent investigation.

Comment 5

[Simon Montagu :smontagu]

Attachment: Stack for the first assertion

Comment 6

[Jesse Ruderman]

The first assertion also shows up in bug 772668, btw.

Comment 7

[Jesse Ruderman]

You can automate the creation of a profile that uses this pref:

mkdir ~/pxa/
echo 'user_pref("bidi.browser.ui", true);' > ~/pxa/prefs.js
firefox -profile ~/pxa/

Comment 8

[Simon Montagu :smontagu]

Attachment: Patch for the crash

I'll attach this patch for the crash in the bidi caret code to get feedback on it before I forget, but as I said in the previous comment, the first assertion probably needs to be addressed first, and I'll leave that to the experts on editor and selection kind of stuff :-)

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Is this going into trunk soon? It will need to have sec-approval? set and the questions there answered.

Since this affects beta and aurora, patches there should be prepared and nominated so we can fix this everywhere?

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 768565 [details] [diff] [review]
Patch for the crash

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The comments and the location of the patch point to the combination of bidi text and text changing, but that doesn't narrow it down very far.

Which older supported branches are affected by this flaw? 23, 24, 25

If not all supported branches, which bug introduced the flaw? Bug 765780 exposed the bug in designMode, but it has probably existed in some form for a long time.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch applies as-is to aurora and beta.

How likely is this patch to cause regressions; how much testing does it need? I think it's not likely to cause regressions, but it could do with testing by someone with experience in editing bidirectional text, with the bidi.ui pref set.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 768565 [details] [diff] [review]
Patch for the crash

Approving for aurora and beta along with sec-approval+. It should go into trunk first but we'll want this in aurora and beta ASAP after that.

Comment 12

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5522f742108

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5522f742108

Comment 14

[Simon Montagu :smontagu]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e65440494bc6
https://hg.mozilla.org/releases/mozilla-beta/rev/2492f2386f35

Comment 15

[Andrew McCreight [:mccr8]]

Looks like this landed on trunk, so I'm marking this fixed.

Comment 16

[Simon Montagu :smontagu]

The assertion "aNode isn't in mRange, or something else weird happened" isn't fixed yet. Does that need a new bug, or is it covered by bug 772668?

Comment 17

[Andrew McCreight [:mccr8]]

Oh, right.  It is probably good to split out the assertion into a new bug just to make tracking the security issue easier.  I don't know if the assertion is different here than in the other bug.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Is Firefox 22 unaffected here?

Comment 19

[Simon Montagu :smontagu]

The bug is not reproducible on trunk builds before 2013-04-09, so Firefox 22 should be unaffected, but I haven't tested.