Closed Bug 1285273 Opened 4 years ago Closed 4 years ago

Crash in nsLayoutUtils::FindNearestCommonAncestorFrame

Categories

(Core :: DOM: Selection, defect)

48 Branch
Unspecified
Android
defect
Not set
critical

Tracking

()

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

People

(Reporter: marcia, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-f2dad13b-3cd3-4615-990f-2aa362160707.
=============================================================

Seen while looking at crash stats - http://bit.ly/29tnzgS. Present in 48/49/50. #7 top crash in 48.0b4

ni on maysauki to see if he has any ideas.
Actually this seems to be solidly a selection bug (as opposed to APZ). From the stack most likely mozilla::AccessibleCaretManager::SelectWordOrShortcut is passing null or some garbage to the nsLayoutUtils functions.
Blocks: gecko-carets
No longer blocks: fennec-aboard-apz
Component: Panning and Zooming → Selection
One guess from looking at the relevant code is that the call to ChangeFocusToOrClearOldFocus triggers arbitrary listeners to run, triggers reflow or relayout or frame reconstruction, and invalidates the rootFrame and/or ptFrame pointers, which are then passed in to TransformPoint.
It seems you forgot to ni? masayuki.
Flags: needinfo?(masayuki)
Actually, this crash report is weird. The source link of "nsLayoutUtils::FindNearestCommonAncestorFrame" doesn't point to that function at all, so it is unknown in which line this happens.
According to bug 1229850 comment 2, 0xf0dea81b seems like an small offset address of a poison frame, so comment #2 might be a valid guess.
Yeah, comment 2 does make sense. And also the call of IMEStateManager::NotifyIME(widget::REQUEST_TO_COMMIT_COMPOSITION) may also cause reflow, etc.

The method should get weak reference to the frame(s) before calling them and check the frame(s) after each call. I'm not sure what we should do there when the frame is gone, though.
Flags: needinfo?(masayuki)
I don't think frames support weak reference... If we add weak reference support to frame, we would bloat every frame for a pointer size, which may or may not be acceptable.

If NotifyIME can trigger frame reconstruction, then the call to ChangeFocusToOrClearOldFocus seems to be vulnerable as well.
nsWeakFrame won't work?
Oh, okay. I forgot that. I was thinking about WeakPtr.
Assignee: nobody → tlin
Check ptFrame is still alive after calling
ChangeFocusToOrClearOldFocus() and IMEStateManager::NotifyIME().

Review commit: https://reviewboard.mozilla.org/r/63370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63370/
Attachment #8769465 - Flags: review?(mats)
masayuki, are you comfortable in reviewing this patch? Just found that mats is on vacation.
Flags: needinfo?(masayuki)
Sure. Although, I'm not formal reviewer around that.
Flags: needinfo?(masayuki)
Comment on attachment 8769465 [details]
Bug 1285273 - Bail out early if ptFrame died in SelectWordOrShortcut().

https://reviewboard.mozilla.org/r/63370/#review60536

If you designed this as so, I agree with this code. But I hope you update the comment.

::: layout/base/AccessibleCaretManager.cpp:556
(Diff revision 1)
> +  // Get ptInFrame here so that we don't need to check whether rootFrame is
> +  // alive later.
> +  nsPoint ptInFrame = aPoint;
> +  nsLayoutUtils::TransformPoint(rootFrame, ptFrame, ptInFrame);

I *guess* this is wrong because IMEStateManager::NotifyIME() and/or ChangeFocusToOrClearOldFocus() may move the frame. Then, ptInFrame might be wrong place.

However, I'm not sure if SelectWord() should work with old frame position or new frame position because the user must expect that works with the old frame position.

How about you? Even if you designed so, I hope you should explain about this issue with the comment here.
Attachment #8769465 - Flags: review+
https://reviewboard.mozilla.org/r/63370/#review60536

> I *guess* this is wrong because IMEStateManager::NotifyIME() and/or ChangeFocusToOrClearOldFocus() may move the frame. Then, ptInFrame might be wrong place.
> 
> However, I'm not sure if SelectWord() should work with old frame position or new frame position because the user must expect that works with the old frame position.
> 
> How about you? Even if you designed so, I hope you should explain about this issue with the comment here.

Sure. I'll update the comment to note that something under the old frame position will be selected. Thank you for the review!
Attached file onfocus.html
I made a simple test case that the div will be moved or removed when it's focused by long pressing on it. Unfortunately, we cannot turn it into a marionette test since marionette test lacks APZ. I test it manually on fennec to ensure browser doesn't crash after applying my patch.
Depends on: AccessibleCaret
No longer depends on: AccessibleCaret
Comment on attachment 8769465 [details]
Bug 1285273 - Bail out early if ptFrame died in SelectWordOrShortcut().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63370/diff/1-2/
Attachment #8769465 - Flags: review?(mats)
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23da74f8393f
Bail out early if ptFrame died in SelectWordOrShortcut(). r=masayuki
https://hg.mozilla.org/mozilla-central/rev/23da74f8393f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8769465 [details]
Bug 1285273 - Bail out early if ptFrame died in SelectWordOrShortcut().

Approval Request Comment
[Feature/regressing bug #]: Text selection on Fennec (AccessibleCaret)
[User impact if declined]: Fennec crashes. See comment 0 which is a top crash.
[Describe test coverage new/current, TreeHerder]: Landed on m-c with manual test in comment 15.
[Risks and why]: Low. Simple null pointer check on frame tree.
[String/UUID change made/needed]: None.
Attachment #8769465 - Flags: approval-mozilla-beta?
Attachment #8769465 - Flags: approval-mozilla-aurora?
Comment on attachment 8769465 [details]
Bug 1285273 - Bail out early if ptFrame died in SelectWordOrShortcut().

This patch fixes a crash. Take it in 48 beta 8 and aurora. Should be in fennec 48 beta 8.
Attachment #8769465 - Flags: approval-mozilla-beta?
Attachment #8769465 - Flags: approval-mozilla-beta+
Attachment #8769465 - Flags: approval-mozilla-aurora?
Attachment #8769465 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.