Closed Bug 1283828 Opened 3 years ago Closed 3 years ago
Crash in ns
Content Utils::Compare Points | mozilla::Accessible Caret Manager::Restrict Caret Dragging Offsets
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
Kan-Ru, thanks for reporting this bug. We should null check |content| in  before passing it to nsContentUtils::ComparePoints.  https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/layout/base/AccessibleCaretManager.cpp#1034
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)
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.
Review commit: https://reviewboard.mozilla.org/r/62088/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/62088/
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
> 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 firstname.lastname@example.org: 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 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 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.
You need to log in before you can comment on or make changes to this bug.