Closed Bug 1370762 Opened 2 years ago Closed 2 years ago

Crash in mozilla::dom::Selection::ToString

Categories

(Core :: Selection, defect, critical)

Unspecified
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: kanru, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.
Flags: needinfo?(tlin)
> 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?
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Flags: needinfo?(tlin) → needinfo?(kchen)
(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()
Flags: needinfo?(kchen)
Comment on attachment 8875591 [details]
Bug 1370762 - Null check GetSelection() before calling Stringify().

https://reviewboard.mozilla.org/r/147014/#review151126
Attachment #8875591 - Flags: review?(mtseng) → review+
(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.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/33765075ea48
Null check GetSelection() before calling Stringify(). r=mtseng
(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.
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 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.
https://hg.mozilla.org/mozilla-central/rev/33765075ea48
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.