1266782 - Update ExtendPhoneNumberSelection() method in AccessibleCaretManager for strong refs to doc, sel

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Mark Capella [:capella]]

See bug 1265750 comment 8 for background...

I've verified that we do generate SelectionEvent notifications to selection listeners in AccessibleCaretManager::ExtendPhoneNumberSelection().

Comment 1

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

Attachment: MozReview Request: Bug 1266782 - Use RefPtr to hold document and selection in ExtendPhoneNumberSelection.

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

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8745938 [details]
MozReview Request: Bug 1266782 - Use RefPtr to hold document and selection in ExtendPhoneNumberSelection.

https://reviewboard.mozilla.org/r/49189/#review46077

Don't we also need an early return if IsTerminated() becomes true inside the loop?
(immediatly after the Modify call probably makes most sense)

I think we also need the same early return after the ExtendPhoneNumberSelection
calls in SelectMoreIfPhoneNumber, at least the first one, because otherwise
mPresShell might by null in the second call and crash when accessing it.

r=mats with those nits addressed as needed

Comment 3

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

https://reviewboard.mozilla.org/r/49189/#review46077

Assuming Modify() will make mPresShell becomes nullptr, we'll need IsTerminated() check. I'll add that in next patch.

How about we add early return for mPresShell in the beginning of ExtendPhoneNumberSelection()? Then both calls to ExtendPhoneNumberSelection() will be safe, and we don't need to worry about other statements accessing mPresShell added after the second ExtendPhoneNumberSelection() call in the future. 

SetSelectionDirection() in SelectMoreIfPhoneNumber() should be safe when mPresShell is nullptr. If mPresShell is nullptr, then GetSelection() is nullptr, too.

Comment 4

[Mats Palmgren (inactive)]

> How about we add early return for mPresShell in the beginning of ExtendPhoneNumberSelection()?

Sure, that makes it even more robust.

Comment 5

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

Comment on attachment 8745938 [details]
MozReview Request: Bug 1266782 - Use RefPtr to hold document and selection in ExtendPhoneNumberSelection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49189/diff/1-2/

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0409b30c4b

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c0409b30c4b