Closed Bug 456944 Opened 11 years ago Closed 11 years ago
Crash when spell checking selection changes
Here's the report: http://crash-stats.mozilla.com/report/index/345e9f28-88d2-11dd-8723-001321b13766 Looks simple -- we probably just need to check to see if the targetNode was null. I'd like to know when it's null, though. http://hg.mozilla.org/mozilla-central/annotate/78beaa82e920/accessible/src/base/nsCaretAccessible.cpp#l330
Any testcase for this?
(In reply to comment #3) > Any testcase for this? Would be great but currently I can't reproduce this and all I have is crash stack.
There is a null check here http://hg.mozilla.org/mozilla-central/annotate/78beaa82e920/accessible/src/base/nsCaretAccessible.cpp#l321 but not before http://hg.mozilla.org/mozilla-central/annotate/78beaa82e920/accessible/src/base/nsCaretAccessible.cpp#l330 Any reason for that?
Eve(In reply to comment #5) > There is a null check here > http://hg.mozilla.org/mozilla-central/annotate/78beaa82e920/accessible/src/base/nsCaretAccessible.cpp#l321 I can suppose focusNode can be null, therefore there is null check here. > but > not before > http://hg.mozilla.org/mozilla-central/annotate/78beaa82e920/accessible/src/base/nsCaretAccessible.cpp#l330 > Any reason for that? here's focus node is not null and it's an element so I do not expect here the focusa-anchor range is broken and focusOffset points into sky.
Hmm, there is actually even one more null check, line 316. So the crash happens apparently because GetChildAt (line 325) returns nsnull. And that is possible if focusOffset points to some non-existing child index. Seems like nsTypedSelection::GetFocusOffset is wrong. By default it assigns nsnull to the result (which is PRInt32!). At least it should assign 0 (tough, in practice nsnull is 0). Anyway, it returns that default value if mAnchorFocusRange is nsnull, and it may return 0 offset (even if focusNode doesn't have child nodes) also if the range is collapsed to focusnode. That is what I guess is happening. So null check is needed. Or what else could be happening?
Alex, any comments?
Sorry for delay. I'm sure you're right GetFocusOffset returns wrong information but it does it because GetFocusNode is wrong. Since this crash happens when we remove all ranges then I think GetFocusNode must return nsnull and we won't ever check GetFocusOffset. I think, DOM is changed and it forces to change spellcheck selection, then they remove all ranges from spellcheck selection but they do not update focusAnchorRange. Possibly there is underlying problem is range is not properly updated on DOM changes. But any way I think we should drop focusAnchorRange when ranges are removed because it seems it doesn't make sense to keep it alive.
Comment on attachment 340737 [details] [diff] [review] patch2 Ok, seems like a possible regression from Bug 337871.
Attachment #340737 - Flags: review?(Olli.Pettay) → review+
(In reply to comment #10) > (From update of attachment 340737 [details] [diff] [review]) > Ok, seems like a possible regression from Bug 337871. Right. Looks so. Thank you for pointing this. There is testcase there, so I'll try to reproduce this bug by it.
Attachment #340737 - Flags: superreview?(roc) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.