Closed Bug 1266782 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Selection, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: capella, Assigned: TYLin)

Details

Attachments

(1 file)

See bug 1265750 comment 8 for background...

I've verified that we do generate SelectionEvent notifications to selection listeners in AccessibleCaretManager::ExtendPhoneNumberSelection().
Assignee: nobody → tlin
Status: NEW → ASSIGNED
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
Attachment #8745938 - Flags: review?(mats) → review+
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.
> How about we add early return for mPresShell in the beginning of ExtendPhoneNumberSelection()?

Sure, that makes it even more robust.
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/
https://hg.mozilla.org/mozilla-central/rev/8c0409b30c4b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.