If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

"ASSERTION: Wrong offset of the DOM point!" and crash in nsAccUtils::GetTextAccessibleFromSelection

RESOLVED WORKSFORME

Status

()

Core
Disability Access APIs
--
critical
RESOLVED WORKSFORME
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Blocks: 2 bugs, {assertion, crash, testcase})

Trunk
assertion, crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 634769 [details]
testcase (requires extension)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase.

###!!! ASSERTION: Wrong offset of the DOM point!: 'aOffset >= 0 && aOffset <= childCount', file accessible/src/base/nsCoreUtils.cpp, line 207

###!!! ABORT: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file nsCOMPtr.h, line 775

[@ nsAccUtils::GetTextAccessibleFromSelection]
(Reporter)

Comment 1

5 years ago
Created attachment 634770 [details]
stack traces

Comment 2

5 years ago
On Windows: bp-6f9e678b-45d7-490f-b95b-690442120620.
Crash Signature: [@ NS_DebugBreak_P | nsCoreUtils::GetDOMNodeFromDOMPoint] [@ nsINode::OwnerDoc()]
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: "ASSERTION: Wrong offset of the DOM point!" and crash → "ASSERTION: Wrong offset of the DOM point!" and crash in nsAccUtils::GetTextAccessibleFromSelection
A thought: maybe GetFocusOffset (nsAccUtils::GetTextAccessibleFromSelection) is giving us the wrong offset. Possibly mAnchorFocusRange is confused by the test case?

Boris what do you think?
(In reply to David Bolter [:davidb] from comment #3)
> A thought: maybe GetFocusOffset (nsAccUtils::GetTextAccessibleFromSelection)
> is giving us the wrong offset. Possibly mAnchorFocusRange is confused by the
> test case?

is there any case in which offset > node.childCount for a dom point?
> is there any case in which offset > node.childCount for a dom point?

In theory, no.  In practice, I think Aryeh recently ran into such cases in editor-land.
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> is there any case in which offset > node.childCount for a dom point?

Yes -- briefly.  When a DOM mutation fires, that synchronously calls various functions that are registered to listen for mutations.  One of those is responsible for adjusting ranges.  Someone else would know the details better, but nsRange is derived from nsIMutationObserver, and the implementation of how it updates its endpoints is in the appropriate nsRange methods (nsRange::CharacterDataChanged, nsRange::ContentAppended, etc.).

So if you remove a node, that will run nsRange::ContentRemoved to decrement the range's offset.  This should happen before any script runs.  But if anything else is also listening for the mutation, it might run before ranges are updated, and it will then have to deal with these bogus ranges.  You might want to arrange it so it runs after ranges are updated -- I don't know how one would go about doing that.

Needless to say, you'll only actually hit this assert if the offset is actually greater than the length.  In all other cases, the range will just be bogus and you won't be able to tell -- the offset will just be at the wrong place because it hasn't been updated yet.  So one thing that is *not* a correct fix is to just special-case the offset > length case, unless you really don't care about hitting the wrong place is other cases.

(In reply to Boris Zbarsky (:bz) (On vacation until July 5) from comment #5)
> In theory, no.  In practice, I think Aryeh recently ran into such cases in
> editor-land.

Bug 767169, for reference.
Thanks Aryeh!

Trevor do you feel like taking this one? I wonder if we can get called just before scripts run?
(In reply to :Aryeh Gregor from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > is there any case in which offset > node.childCount for a dom point?
> 
> Yes -- briefly.  When a DOM mutation fires, that synchronously calls various
> functions that are registered to listen for mutations.  One of those is
> responsible for adjusting ranges.  Someone else would know the details
> better, but nsRange is derived from nsIMutationObserver, and the
> implementation of how it updates its endpoints is in the appropriate nsRange
> methods (nsRange::CharacterDataChanged, nsRange::ContentAppended, etc.).
> 
> So if you remove a node, that will run nsRange::ContentRemoved to decrement
> the range's offset.  This should happen before any script runs.  But if
> anything else is also listening for the mutation, it might run before ranges
> are updated, and it will then have to deal with these bogus ranges.  You
> might want to arrange it so it runs after ranges are updated -- I don't know
> how one would go about doing that.

well, the code that is running into this is being called by the refresh driver.  I'm pretty sure that at that point the sync processing of the DOM mutation should be over.
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> (In reply to :Aryeh Gregor from comment #6)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > > is there any case in which offset > node.childCount for a dom point?
> > 
> > Yes -- briefly.  When a DOM mutation fires, that synchronously calls various
> > functions that are registered to listen for mutations.  One of those is
> > responsible for adjusting ranges.  Someone else would know the details
> > better, but nsRange is derived from nsIMutationObserver, and the
> > implementation of how it updates its endpoints is in the appropriate nsRange
> > methods (nsRange::CharacterDataChanged, nsRange::ContentAppended, etc.).
> > 
> > So if you remove a node, that will run nsRange::ContentRemoved to decrement
> > the range's offset.  This should happen before any script runs.  But if
> > anything else is also listening for the mutation, it might run before ranges
> > are updated, and it will then have to deal with these bogus ranges.  You
> > might want to arrange it so it runs after ranges are updated -- I don't know
> > how one would go about doing that.
> 
> well, the code that is running into this is being called by the refresh
> driver.  I'm pretty sure that at that point the sync processing of the DOM
> mutation should be over.

Boris is that true?
> Boris is that true?

Yes.

Comment 11

5 years ago
It doesn't crash with me.

1) running testcase having the extension installed I get error in console:
Timestamp: 9/4/2012 9:42:48 AM
Error: The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol.
Source File: file:///Z:/alex%20On%20My%20Mac/mozilla/tests/crash766471.html
Line: 0
2) enabling a11y (running DOMi) I don't get crash as well

Can you confirm the bug is still here please?
(Reporter)

Comment 12

5 years ago
WFM on mozilla-central.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.