Closed Bug 456944 Opened 11 years ago Closed 11 years ago

Crash when spell checking selection changes

Categories

(Core :: Disability Access APIs, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: surkov)

Details

(Keywords: access, crash)

Attachments

(1 file, 1 obsolete file)

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
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #340735 - Flags: superreview?(roc)
Attachment #340735 - Flags: review?(Olli.Pettay)
Attached patch patch2Splinter Review
Attachment #340735 - Attachment is obsolete: true
Attachment #340735 - Flags: superreview?(roc)
Attachment #340735 - Flags: review?(Olli.Pettay)
Attachment #340737 - Flags: superreview?(roc)
Attachment #340737 - Flags: review?(Olli.Pettay)
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.
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+
Flags: in-testsuite?
checked in, http://hg.mozilla.org/mozilla-central/rev/681e49004d91
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.