1285273 - Crash in nsLayoutUtils::FindNearestCommonAncestorFrame

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Marcia Knous [:marcia]]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

It seems you forgot to ni? masayuki.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsWeakFrame won't work?

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Oh, okay. I forgot that. I was thinking about WeakPtr.

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1285273 - Bail out early if ptFrame died in SelectWordOrShortcut().

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/

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

masayuki, are you comfortable in reviewing this patch? Just found that mats is on vacation.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sure. Although, I'm not formal reviewer around that.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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!

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 16

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23da74f8393f
Bail out early if ptFrame died in SelectWordOrShortcut(). r=masayuki

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23da74f8393f

Comment 19

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.

Comment 20

[Gerry Chang [:gchang]]

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.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/709652b67401

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/84c59d136a9e