Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 362428

Summary:	_getXToOffset throws: index out of range
Product:	[ECD] Orion	Reporter:	Mihai Sucan <mihai.sucan>
Component:	Editor	Assignee:	Mihai Sucan <mihai.sucan>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	eclipse.felipe, mihai.sucan, Silenio_Quarti
Version:	unspecified
Target Milestone:	0.4 M1
Hardware:	All
OS:	All
Whiteboard:

Description Mihai Sucan

2011-10-30 06:52:15 EDT

While writing an automated test for bug 360672 using Mozilla's mochitest harness I found a bug.

The TextView._getXToOffset() method can give out of range indexes to the browser.

The automated test kept on failing until I saw this:

Error: uncaught exception: [Exception... "Index or size is negative or greater than the allowed amount"  code: "1" nsresult: "0x80530001 (NS_ERROR_DOM_INDEX_SIZE_ERR)"  location: "chrome://browser/content/orion.js Line: 6088"]

That's line 3995:

  range.setEnd(textNode, end);

But the problem is at line 3992:

  end = high === nodeLength - 1 && lineChild.ignoreChars ? textNode.length : high + 1;

The case I was hitting was that |high| was already == nodeLength, so |end| was set to |high + 1|, which is an out-of-range index.

If you look into the code above, |high| starts with |high = nodeLength|. If |x| is not |found| within the first loop through the |rects| of the |range|, then |high| remains the same (|nodeLength|). This is why |end| becomes |high + 1| which effectively means |nodeLength + 1|. This causes the |range.setEnd()| call to throw.

Comment 1 Mihai Sucan

2011-10-30 06:57:57 EDT

Proposed fix:

https://github.com/mihaisucan/orion.client/tree/bug-362428

Please review the patch and let me know if this needs further changes before it can land. Thank you!

Comment 2 Silenio Quarti

2011-11-02 10:57:41 EDT

Hi Mihai, how can I reproduce the problem using the editor ?

I thought that high could never get to nodeLength.
Note that before we start the binary search we test that the x is within the element:
if (rect.left <= x && x < rect.right) {}

Therefore I would expect to always find a offset within the element (between 0 and nodeLength-1) that contains x.

If this code is failing, it possibly means that firefox has a inconsistency between element.getClientRects() and range.getClientRects(); or textview has a bug mapping coordinates or caching the values.


At the end, I think your fix is the right code, but I would like to debug this case to understand the details.

Comment 3 Mihai Sucan

2011-11-02 11:42:17 EDT

Hello Silenio!

Thank you for looking into my patch!


(In reply to comment #2)
> Hi Mihai, how can I reproduce the problem using the editor ?

I cannot reproduce the problem using the editor.

I only see this problem while running the automated Mochitests from the Firefox codebase. If you want to do this you need to get the Firefox source and make a build to run the tests on your machine. I can help you with this - ping me on IRC.


> I thought that high could never get to nodeLength.
> Note that before we start the binary search we test that the x is within the
> element:
> if (rect.left <= x && x < rect.right) {}
> 
> Therefore I would expect to always find a offset within the element (between 0
> and nodeLength-1) that contains x.
> 
> If this code is failing, it possibly means that firefox has a inconsistency
> between element.getClientRects() and range.getClientRects(); or textview has a
> bug mapping coordinates or caching the values.

Agreed.

During the really-fast execution of the automated tests the client rects probably change and cause confusion inside the getXToOffset() method.

I was quite surprised to see this. The cached client rects are probably the problem. I ran the test and halted it mid-run and started using the editor myself. It had the same errors, even during normal usage. This was because of the caching.

This is why it's probably best to have a safeguard against such issues.


> At the end, I think your fix is the right code, but I would like to debug this
> case to understand the details.

This fix could probably be improved by clearing the cached client rects for the line children, when |high| is out of range.

Comment 4 Silenio Quarti

2011-11-02 12:46:02 EDT

any clue why the cache is wrong ?

- the scroll is not up-to-date ?
- the size of the element actually changed ?

Note that we also use the cache in the middle of binary search, couldn't the problem happen there too ?



(this is Felipe)

Comment 5 Mihai Sucan

2011-11-03 08:45:22 EDT

(In reply to comment #4)
> any clue why the cache is wrong ?
> 
> - the scroll is not up-to-date ?
> - the size of the element actually changed ?

Might be both. On-screen I do not even get the chance to see the browser window fully opening before the test is done.

When we work with mochitests for Firefox code it happens sometimes that rendering doesn't happen as soon as the test code needs - we always have to listen to the correct events, to make sure everything we want has happened before executing parts of tests. We sometimes find edge cases, and this is one of those.


> Note that we also use the cache in the middle of binary search, couldn't the
> problem happen there too ?

It could, but my principle was: don't touch code that works. The error only happened where I had to fix it.

This is probably an edge case caused by the sync init of Orion.

Comment 6 Felipe Heidrich

2011-11-04 10:38:03 EDT

(In reply to comment #5)
> This is probably an edge case caused by the sync init of Orion.

Silenio and I are trying to finish the async work that we started and never finished. I hope the code can land today, it will be interesting to try the tests with the async init.

That said, I don't mind if you push your one-line fix to master.

Comment 7 Mihai Sucan

2011-11-04 12:46:53 EDT

(In reply to comment #6)
> (In reply to comment #5)
> > This is probably an edge case caused by the sync init of Orion.
> 
> Silenio and I are trying to finish the async work that we started and never
> finished. I hope the code can land today, it will be interesting to try the
> tests with the async init.

True. Thank you very much for your work!

> That said, I don't mind if you push your one-line fix to master.

Thanks!

Pushed:

http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=7d62ad28320641c408cce24de1eb01f1544db9d3

Comment 8 Mihai Sucan

2011-11-04 17:33:14 EDT

For future reference, this fix has landed in the Firefox codebase some time ago:

https://bugzilla.mozilla.org/show_bug.cgi?id=697407