Crash in nsContentUtils::ComparePoints | mozilla::AccessibleCaretManager::RestrictCaretDraggingOffsets

RESOLVED FIXED in Firefox 48

Status

()

--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kanru, Assigned: TYLin)

Tracking

(Blocks: 1 bug, {crash, regression})

48 Branch
mozilla50
Unspecified
Android
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed, firefox50 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-d8b60044-8de6-416a-8257-b33d92160628.
=============================================================

The crashes started to appear after 2016-04-22 on Fennec. Most of them have mozilla::AccessibleCaretManager::RestrictCaretDraggingOffsets in the stack.

TYLin, I suspect this is a regression from bug 1168891 which landed on m-c on 2016-04-14.

Crash query:
https://crash-stats.mozilla.com/signature/?product=FennecAndroid&platform=Android&date=%3E%3D2016-01-01&signature=nsContentUtils%3A%3AComparePoints&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#reports
Flags: needinfo?(tlin)
(Assignee)

Comment 1

2 years ago
Kan-Ru, thanks for reporting this bug. 

We should null check |content| in [1] before passing it to nsContentUtils::ComparePoints.

[1] https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/layout/base/AccessibleCaretManager.cpp#1034
Assignee: nobody → tlin
Status: NEW → ASSIGNED
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
Flags: needinfo?(tlin)
(Assignee)

Comment 2

2 years ago
Created attachment 8767475 [details]
Bug 1283828 - Null check content in RestrictCaretDraggingOffsets.

Passing null content to nsContentUtils::ComparePoints will lead to a
crash.

Review commit: https://reviewboard.mozilla.org/r/61952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61952/
Attachment #8767475 - Flags: review?(mats)

Updated

2 years ago
Attachment #8767475 - Flags: review?(mats) → review-
Comment on attachment 8767475 [details]
Bug 1283828 - Null check content in RestrictCaretDraggingOffsets.

https://reviewboard.mozilla.org/r/61952/#review58698

Hmm, it seems odd that we would get a frame, with a node that
is either null or doesn't QI to nsIContent.  That seems like
a bogus result from GetFrameForFirstRangeStartOrLastRangeEnd.

Looking at the code:
http://searchfox.org/mozilla-central/source/layout/base/AccessibleCaretManager.cpp#942
Let's say we have aDirection == eDirNext and the startContent+offset
is an element with display:none, then we'll get null from
GetFrameForNodeOffset at line 978, and if startContent also
doesn't have a frame (line 998) then we fall through to the loop
at the end, where we look for the first node in the range that
has a frame instead.  Let's say we find a frame here, we exit
the loop and return it -- but we never assigned aOutNode on
this path.  That seems like a bug; I think it should assign
*aOutNode = startNode and *aOutNodeOffset to something...

So, if you agree with that, then we should probably move the whole
"if (startFrame)" block from line 980 to before the return, and
wrap line 990..1009 in a "if (!startFrame)".  Perhaps we should
also add another "startFrame = fs->GetFrameForNodeOffset" call
after the loop to get the correct aOutOffset?
Dunno what 'nodeOffset' to use though, probably zero for
eDirNext and the last valid index for eDirPrevious.

WDYT?
It should be possible to add a range to the selection that would
take the described path.  I'm not sure what command would trigger
the crash after that though.
(Assignee)

Comment 5

2 years ago
Re comment 3: 

I think you're right that we should properly set *aOutNode and *aOutNodeOffset if the logic goes to the TreeWalker part.  It's bad that GetFrameForFirstRangeStartOrLastRangeEnd returns a frame with nullptr node and incorrect offset.  Perhaps we could use 0 for both aOutOffset and the aOutNodeOffset since we are walking in the content tree.

I could not find an STR to reproduce the crash, but we should at least ensure *aOutNode and *aOutNodeOffset are set in GetFrameForFirstRangeStartOrLastRangeEnd() if a non-null frame is returned.
(Assignee)

Comment 6

2 years ago
Created attachment 8767623 [details]
Bug 1283828 - Ensure output arguments are set in GetFrameForFirstRangeStartOrLastRangeEnd if returned frame isn't nullptr.

Review commit: https://reviewboard.mozilla.org/r/62088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62088/
(Assignee)

Updated

2 years ago
Attachment #8767475 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8767623 - Flags: review?(mats)
(Assignee)

Comment 7

2 years ago
BTW, I don't feel calling startFrame = fs->GetFrameForNodeOffset() after the loop will 100% get the correct frame and frame offset.  When we fall though the TreeWalker logic, the node offset will be artificial (set to 0), we might end up getting a null startFrame when calling fs->GetFrameForNodeOffset().
https://reviewboard.mozilla.org/r/62088/#review58878

> When we fall though the TreeWalker logic, the node offset will be artificial

Yes, that's why I suggested using zero for eDirNext and GetChildCount()
for eDirPrevious, to get the start/end position for startNode.

> we might end up getting a null startFrame when calling
> fs->GetFrameForNodeOffset().

True, but we could fall back to just using startFrame with a zero
index in that case, like you do in this patch.

Anyway, this should fix the crash so let's take this for now.  We can
improve this edge case later if there's a use case that needs it.
r=mats

Updated

2 years ago
Attachment #8767623 - Flags: review?(mats) → review+
(Assignee)

Comment 9

2 years ago
> Anyway, this should fix the crash so let's take this for now.  We can
> improve this edge case later if there's a use case that needs it.
> r=mats

Yes. Let's land this to fix the crash.  I guess the edge case is rare.  Otherwise we'll see the crash more often.  Thank you for the review!

Comment 10

2 years ago
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb45e03a6c2e
Ensure output arguments are set in GetFrameForFirstRangeStartOrLastRangeEnd if returned frame isn't nullptr. r=mats

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fb45e03a6c2e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Comment 12

2 years ago
Comment on attachment 8767623 [details]
Bug 1283828 - Ensure output arguments are set in GetFrameForFirstRangeStartOrLastRangeEnd if returned frame isn't nullptr.

Approval Request Comment
[Feature/regressing bug #]: AccessibleCaret and text selection on Fennec regressing by Bug 1168891.
[User impact if declined]: Fennec crashes.
[Describe test coverage new/current, TreeHerder]: All test pass on Nightly.
[Risks and why]: Low. Simple logic reorder to prevent nullptr.
[String/UUID change made/needed]: none
Attachment #8767623 - Flags: approval-mozilla-beta?
Attachment #8767623 - Flags: approval-mozilla-aurora?
Comment on attachment 8767623 [details]
Bug 1283828 - Ensure output arguments are set in GetFrameForFirstRangeStartOrLastRangeEnd if returned frame isn't nullptr.

This fixes a crash regression. Take it in 48 beta 6 and aurora.
Attachment #8767623 - Flags: approval-mozilla-beta?
Attachment #8767623 - Flags: approval-mozilla-beta+
Attachment #8767623 - Flags: approval-mozilla-aurora?
Attachment #8767623 - Flags: approval-mozilla-aurora+

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b83c0bda78d
status-firefox49: affected → fixed

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/01a1714a1d04
status-firefox48: affected → fixed
Version: unspecified → 48 Branch
(Assignee)

Updated

2 years ago
Blocks: 1124074
You need to log in before you can comment on or make changes to this bug.