1370762 - Crash in mozilla::dom::Selection::ToString

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

This bug was filed from the Socorro interface and is 
report bp-e4f77a45-6549-4be8-b733-32e7d0170606.
=============================================================

30 crashes on FennecAndroid in the past 3 months.

mozilla::dom::Selection::ToString
mozilla::AccessibleCaretManager::StringifiedSelection
mozilla::AccessibleCaretManager::SelectMoreIfPhoneNumber
mozilla::AccessibleCaretManager::SelectWord
mozilla::AccessibleCaretManager::SelectWordOrShortcut
mozilla::AccessibleCaretEventHub::LongTapState::OnLongTap
mozilla::AccessibleCaretEventHub::HandleMouseEvent
mozilla::PresShell::HandleEvent
nsViewManager::DispatchEvent
nsView::HandleEvent
nsWindow::DispatchEvent
mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent
mozilla::layers::APZCCallbackHelper::DispatchSynthesizedMouseEvent
mozilla::layers::APZEventState::FireContextmenuEvents
mozilla::layers::APZEventState::ProcessLongTap
mozilla::layers::ChromeProcessController::HandleTap
mozilla::detail::RunnableMethodImpl<T>::Run
nsThread::ProcessNextEvent
NS_ProcessNextEvent
mozilla::ipc::MessagePump::Run
MessageLoop::Run
nsBaseAppShell::Run
nsAppStartup::Run
XREMain::XRE_mainRun

Missing a null check? Please consider to wrap the return value with mozilla::Maybe.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

> Missing a null check? Please consider to wrap the return value with mozilla::Maybe.

Yes. I think we'll need to check whether GetSelection() is nullptr or not.  Which return value do you mean here?

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #1)
> > Missing a null check? Please consider to wrap the return value with mozilla::Maybe.
> 
> Yes. I think we'll need to check whether GetSelection() is nullptr or not. 
> Which return value do you mean here?

I mean the return value of GetSelection()

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1370762 - Null check GetSelection() before calling Stringify().

Review commit: https://reviewboard.mozilla.org/r/147014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147014/

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8875591 [details]
Bug 1370762 - Null check GetSelection() before calling Stringify().

https://reviewboard.mozilla.org/r/147014/#review151126

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #2)
> I mean the return value of GetSelection()

In gecko, it seems unusual to wrap a pointer in mozilla::Maybe instead of checking the validity of the pointer.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33765075ea48
Null check GetSelection() before calling Stringify(). r=mtseng

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #5)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #2)
> > I mean the return value of GetSelection()
> 
> In gecko, it seems unusual to wrap a pointer in mozilla::Maybe instead of
> checking the validity of the pointer.

Yes, and I consider that a bad practice ;-)

We have MOZ_MUST_USE to enforce that a return value must be used. We have mozilla::Result to enforce that a fallible function's result are checked. I think mozilla::Maybe should be used when a return value is nullable.

Anyway, thanks for fixing this.

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Re comment 7:
> I think mozilla::Maybe should be used when a return value is nullable.

I guess this means when mozilla::Maybe wraps a pointer type, the responsibility of null check is on the callee side, which must wrap the non-nullptr pointer in Some.

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #8)
> Re comment 7:
> > I think mozilla::Maybe should be used when a return value is nullable.
> 
> I guess this means when mozilla::Maybe wraps a pointer type, the
> responsibility of null check is on the callee side, which must wrap the
> non-nullptr pointer in Some.

In this case instead of return nullptr, Nothing() is returned.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33765075ea48