Closed
Bug 1285273
Opened 8 years ago
Closed 8 years ago
Crash in nsLayoutUtils::FindNearestCommonAncestorFrame
Categories
(Core :: DOM: Selection, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: marcia, Assigned: TYLin)
References
(Blocks 1 open bug)
Details
(Keywords: crash)
Crash Data
Attachments
(2 files)
(deleted),
text/x-review-board-request
|
masayuki
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details |
(deleted),
text/html
|
Details |
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.
Updated•8 years ago
|
Blocks: fennec-aboard-apz
Comment 1•8 years ago
|
||
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.
Component: Panning and Zooming → Selection
Comment 2•8 years ago
|
||
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 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
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•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
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•8 years ago
|
||
nsWeakFrame won't work?
Comment 9•8 years ago
|
||
Oh, okay. I forgot that. I was thinking about WeakPtr.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → tlin
Assignee | ||
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
masayuki, are you comfortable in reviewing this patch? Just found that mats is on vacation.
Flags: needinfo?(masayuki)
Comment 12•8 years ago
|
||
Sure. Although, I'm not formal reviewer around that.
Flags: needinfo?(masayuki)
Comment 13•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
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!
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Depends on: AccessibleCaret
Assignee | ||
Updated•8 years ago
|
Blocks: AccessibleCaret
No longer depends on: AccessibleCaret
Assignee | ||
Comment 16•8 years ago
|
||
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)
Comment 17•8 years ago
|
||
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23da74f8393f
Bail out early if ptFrame died in SelectWordOrShortcut(). r=masayuki
Comment 18•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 19•8 years ago
|
||
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 20•8 years ago
|
||
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+
Comment 21•8 years ago
|
||
bugherder uplift |
Comment 22•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•