Closed Bug 1283828 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Selection, defect, critical)

48 Branch
Unspecified
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- fixed
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: kanru, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 1 obsolete file)

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)
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
Flags: needinfo?(tlin)
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)
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.
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.
Attachment #8767475 - Attachment is obsolete: true
Attachment #8767623 - Flags: review?(mats)
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
Attachment #8767623 - Flags: review?(mats) → review+
> 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!
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
https://hg.mozilla.org/mozilla-central/rev/fb45e03a6c2e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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+
Version: unspecified → 48 Branch
You need to log in before you can comment on or make changes to this bug.