Closed Bug 699353 Opened 10 years ago Closed 10 years ago

"ASSERTION: aOffset must be in the frame's range" with caret browsing

Categories

(Core :: DOM: Editor, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: jruderman, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 4 obsolete files)

With
  user_pref("accessibility.browsewithcaret", true);
the testcase triggers:

###!!! ASSERTION: aOffset must be in the frame's range: 'aOffset >= contentOffset && aOffset <= contentOffset + contentLength', file layout/generic/nsTextFrameThebes.cpp, line 5122
Attached file stack traces
nsTextFrame::GetChildFrameContainingOffset will happily return offsets that are outside of a frame's offset range.  I don't know why, but is that something which should be fixed?
That should only happen if the offset is beyond the text node length. Is that the situation here?
Yes.  aOffset is 2, the text node length is 1.
I reckon we should leave that code as-is, assert that the offset is within the text node length, and fix any callers that aren't doing that.
Attached patch Patch (v1) (obsolete) — Splinter Review
Due to the fact that this test case requires caret browsing, I don't think there is a good way for us to import it as a crashtest...
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #589924 - Flags: review?(roc)
> Due to the fact that this test case requires caret browsing, I don't think
> there is a good way for us to import it as a crashtest...

Try this in the crashtest manifest:

   pref(accessibility.browsewithcaret,true) load ...
(In reply to Jesse Ruderman from comment #7)
> > Due to the fact that this test case requires caret browsing, I don't think
> > there is a good way for us to import it as a crashtest...
> 
> Try this in the crashtest manifest:
> 
>    pref(accessibility.browsewithcaret,true) load ...

Neat!
Attached patch Test case (obsolete) — Splinter Review
Attachment #590320 - Flags: review?(roc)
Attached patch Patch for landing (obsolete) — Splinter Review
Attachment #589924 - Attachment is obsolete: true
Attachment #590320 - Attachment is obsolete: true
Attached patch Patch for landing (obsolete) — Splinter Review
Attachment #591926 - Attachment is obsolete: true
Whiteboard: [autoland]
Whiteboard: [autoland] → [autoland-in-queue]
Whiteboard: [autoland-in-queue] → [autoland]
Whiteboard: [autoland] → [autoland-in-queue]
Autoland Patchset:
	Patches: 591927
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Try run started, revision 5ad14c5b0d1a. To cancel or monitor the job, see: https://build.mozilla.org/buildapi/self-serve/try/rev/5ad14c5b0d1a
Try run for 5ad14c5b0d1a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5ad14c5b0d1a
Results (out of 208 total builds):
    exception: 2
    success: 174
    warnings: 32
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-5ad14c5b0d1a
Whiteboard: [autoland-in-queue]
Only do the check for text frames, and also fix a test issue which was causing orange.
Attachment #591927 - Attachment is obsolete: true
Attachment #593207 - Flags: review?(roc)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dcf885c2139
Flags: in-testsuite+
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/7dcf885c2139
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.