388175 - Crash [@ IsBidiLeaf] with contentEditable, tables, dir=rtl

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Thread 0 Crashed:
0   IsBidiLeaf(nsIFrame*) + 9 (nsBidiPresUtils.cpp:473)
1   nsBidiPresUtils::GetFrameEmbeddingLevel(nsIFrame*) + 60 (nsBidiPresUtils.cpp:706)
2   nsIFrame::PeekOffset(nsPeekOffsetStruct*) + 2887 (nsFrame.cpp:4991)
3   nsCaret::GetCaretFrameForNodeOffset(nsIContent*, int, nsFrameSelection::HINT, unsigned char, nsIFrame**, int*) + 1186 (nsCaret.cpp:714)
4   nsCaret::DrawAtPositionWithHint(nsIDOMNode*, int, nsFrameSelection::HINT, unsigned char, int) + 169 (nsCaret.cpp:584)
5   nsCaret::DrawCaret(int) + 816 (nsCaret.cpp:980)
6   nsCaret::StartBlinking() + 97 (nsCaret.cpp:553)
7   nsCaret::NotifySelectionChanged(nsIDOMDocument*, nsISelection*, short) + 166 (nsCaret.cpp:494)
8   nsTypedSelection::NotifySelectionListeners() + 407 (nsSelection.cpp:7407)
9   nsFrameSelection::NotifySelectionListeners(short) + 50 (nsSelection.cpp:2845)
10  nsFrameSelection::TakeFocus(nsIContent*, unsigned, unsigned, int, int) + 1823 (nsSelection.cpp:2486)
11  nsFrameSelection::SetAncestorLimiter(nsIContent*) + 179 (nsSelection.cpp:3857)
12  nsTypedSelection::SetAncestorLimiter(nsIContent*) + 27 (nsSelection.cpp:5331)
13  nsTextEditorFocusListener::Focus(nsIDOMEvent*) + 1071 (nsEditorEventListeners.cpp:1151)
14  DispatchToInterface(nsIDOMEvent*, nsIDOMEventListener*, unsigned (nsIDOMEventListener::*)(nsIDOMEvent*), nsID const&) + 189 (nsEventListenerManager.cpp:182)
15  nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsISupports*, unsigned, nsEventStatus*) + 1174 (nsEventListenerManager.cpp:1207)
16  nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned) + 316 (nsEventDispatcher.cpp:202)
17  nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned, nsDispatchingCallback*) + 191 (nsEventDispatcher.cpp:234)
18  nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*) + 1407 (nsEventDispatcher.cpp:473)
19  nsEventStateManager::SendFocusBlur(nsPresContext*, nsIContent*, int) + 3708 (nsEventStateManager.cpp:4366)
20  nsEventStateManager::SetContentState(nsIContent*, int) + 1461 (nsEventStateManager.cpp:3928)
21  nsGenericElement::SetFocus(nsPresContext*) + 178 (nsGenericElement.cpp:2451)
22  nsGenericHTMLElement::SetElementFocus(int) + 154 (nsGenericHTMLElement.cpp:3262)
23  nsGenericHTMLElement::Focus() + 45 (nsGenericHTMLElement.cpp:3289)
24  nsGenericHTMLElementTearoff::Focus() + 36 (nsGenericHTMLElement.cpp:196)
25  NS_InvokeByIndex_P + 98 (xptcinvoke_unixish_x86.cpp:179)
...

Comment 1

[Uri Bernstein]

This is very similar to Bug 345616. I'll take care of it sometime this week, when I have time.

Comment 2

[Jesse Ruderman]

Uri, are you going to be able to fix this soon?

Comment 3

[Uri Bernstein]

Attachment: trivial fix

This fixes the crash, although it's probably not the best way to do it.  I haven't posted the trivial fix until now because I was hoping to find time to investigate further and come up with a more correct solution (such as actually implementing nsTableRowGroupFrame::CheckLineOrder()). Unfortunately, I can't find time for that.

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 277113 [details] [diff] [review]
trivial fix

Can you file a follow-up bug on implementing the more correct solution?

Comment 5

[Uri Bernstein]

(In reply to comment #4)
> (From update of attachment 277113 [details] [diff] [review])
> Can you file a follow-up bug on implementing the more correct solution?
> 

Bug 392706.

Comment 6

[Uri Bernstein]

Sorry, I'm not up to date on the current approval rules. Do I need to ask approval on this?
In any case, I won't be able to check this in myself for the next two weeks or so, so I'd appreciate it if someone can do this for me.

Comment 7

[Simon Montagu :smontagu]

You do need to ask approval at the moment.

Comment 8

[Uri Bernstein]

I guess I should also justify the approval request:
This is a simple, very low risk, fix to a crash (although probably a very rare one).

Comment 9

[Jesse Ruderman]

It's not very rare when I'm trying to test contentEditable ;)

Comment 10

[Simon Montagu :smontagu]

Checked in.

Comment 11

[Jesse Ruderman]

Crashtest checked in.

Comment 12

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).