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

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: 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/

Comment 3

[Mats Palmgren (inactive)]

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?

Comment 4

[Mats Palmgren (inactive)]

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.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: 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/

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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().

Comment 8

[Mats Palmgren (inactive)]

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

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

> 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

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb45e03a6c2e

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 13

[Gerry Chang [:gchang]]

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.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b83c0bda78d

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/01a1714a1d04