1457862 - Crash in mozilla::EditorBase::GetChildOffset

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-7378c60d-ae08-4521-81d4-11aa00180427.
=============================================================

Seen while looking at nightly crash stats - small volume Windows crash that started using 20180427102245: https://bit.ly/2vXfyxH

Looks as if some code was touched in Bug 1455674. ni on :bz

Top 10 frames of crashing thread:

0 xul.dll mozilla::EditorBase::GetChildOffset editor/libeditor/EditorBase.cpp:3376
1 xul.dll mozilla::HTMLEditor::GetCellContext editor/libeditor/HTMLTableEditor.cpp:2843
2 xul.dll mozilla::HTMLEditor::InsertTableCell editor/libeditor/HTMLTableEditor.cpp:178
3 xul.dll mozilla::HTMLEditor::InsertTableColumn editor/libeditor/HTMLTableEditor.cpp:437
4 xul.dll mozilla::HTMLEditor::DoInlineTableEditingAction editor/libeditor/HTMLInlineTableEditor.cpp:143
5 xul.dll mozilla::HTMLEditorEventListener::MouseClick editor/libeditor/HTMLEditorEventListener.cpp:202
6 xul.dll mozilla::EditorEventListener::HandleEvent editor/libeditor/EditorEventListener.cpp:458
7 xul.dll mozilla::EventListenerManager::HandleEventSubType dom/events/EventListenerManager.cpp:1121
8 xul.dll mozilla::EventListenerManager::HandleEventInternal dom/events/EventListenerManager.cpp:1288
9 xul.dll mozilla::EventTargetChainItem::HandleEventTargetChain dom/events/EventDispatcher.cpp:528

=============================================================

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't forget cellParent until we're done using it in HTMLEditor::GetCellContext

Any ideas on how to write a test for this?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Marcia, thank you for filing this!

Looks like a null deref of aParent when doing aParent->GetFirstChild() in EditorBase::GetChildOffset.  The problem is this code in GetCellContext:


    nsCOMPtr<nsINode> cellParent = cell->GetParentNode();

    // Cell has to have a parent, so fail if not found
    NS_ENSURE_TRUE(cellParent, NS_ERROR_FAILURE);

    cellParent.forget(aCellParent);

    if (aCellOffset) {
      *aCellOffset = GetChildOffset(cell, cellParent);

    }

which nulls out cellParent (via the forget() call) after the null-check.  This was indeed changed in bug 1455674; it was OK before.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8972100 [details] [diff] [review]
Don't forget cellParent until we're done using it in HTMLEditor::GetCellContext

Folks in Japan are on holiday.


The old code did
-    *aCellParent = cellParent->AsDOMNode();
-    NS_ADDREF(*aCellParent);
so the change looks reasonable.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0fcc5bcaf24
Don't forget cellParent until we're done using it in HTMLEditor::GetCellContext.  r=masayuki

Comment 5

[Andreea Pavel [:apavel]]

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, smaug.