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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires focus, requires browse-with-caret pref)

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

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[(no longer active)]

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?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That should only happen if the offset is beyond the text node length. Is that the situation here?

Comment 4

[(no longer active)]

Yes.  aOffset is 2, the text node length is 1.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

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...

Comment 7

[Jesse Ruderman]

> 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 ...

Comment 8

[(no longer active)]

(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!

Comment 9

[(no longer active)]

Attachment: Test case

Comment 10

[(no longer active)]

Attachment: Patch for landing

Comment 11

[(no longer active)]

Attachment: Patch for landing

Comment 12

[Mozilla RelEng Bot]

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

Comment 13

[Mozilla RelEng Bot]

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/[e-mail]-5ad14c5b0d1a

Comment 14

[(no longer active)]

Attachment: Fixed a bunch of test issues

Only do the check for text frames, and also fix a test issue which was causing orange.

Comment 15

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dcf885c2139

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7dcf885c2139