637898 - ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file ../../../gfx/thebes/gfxSkipChars.cpp, line 92

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Mats Palmgren (inactive)]

(Rev3 Fedora 12 tryserver debug test mochitest-other on 2011/02/25 20:08:09)

chrome://mochitests/content/a11y/accessible/editabletext/test_1.html
triggered this assertion twice:

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file ../../../gfx/thebes/gfxSkipChars.cpp, line 92
gfxSkipCharsIterator::SetOffsets [gfxSkipChars.cpp:94]
gfxSkipCharsIterator::SetSkippedOffset [gfxSkipChars.h:275]
gfxSkipCharsIterator::ConvertSkippedToOriginal [gfxSkipChars.h:283]
nsHyperTextAccessible::RenderedToContentOffset [nsHyperTextAccessible.cpp:2165]
nsHyperTextAccessible::GetDOMPointByFrameOffset [nsHyperTextAccessible.cpp:2304]
nsHyperTextAccessible::HypertextOffsetsToDOMRange [nsHyperTextAccessible.cpp:770]
nsHyperTextAccessible::SetSelectionBounds [nsHyperTextAccessible.cpp:1945]
nsHyperTextAccessible::SetSelectionRange [nsHyperTextAccessible.cpp:1598]
nsHyperTextAccessible::DeleteText [nsHyperTextAccessible.cpp:1525]
nsHyperTextAccessible::SetTextContents [nsHyperTextAccessible.cpp:1471]
xptiInterfaceEntry::EnsureResolved [xptiprivate.h:264]
CallMethodHelper::Invoke [xpcwrappednative.cpp:3124]
CallMethodHelper::Call [xpcwrappednative.cpp:2390]
XPCWrappedNative::CallMethod [xpcwrappednative.cpp:2354]
XPC_WN_CallMethod [xpcwrappednativejsops.cpp:1613]
js::CallJSNative [jscntxtinlines.h:701]
js::Interpret [jsinterp.cpp:4798]
js::RunScript [jsinterp.cpp:653]
js::Invoke [jsinterp.cpp:740]
js::ExternalInvoke [jsinterp.cpp:863]
JS_CallFunctionValue [jsapi.cpp:5173]
nsXPCWrappedJSClass::CallMethod [xpcwrappedjsclass.cpp:1672]
nsXPCWrappedJS::CallMethod [xpcwrappedjs.cpp:588]
PrepareAndDispatch [xptcstubs_gcc_x86_unix.cpp:95]
nsThread::ProcessNextEvent [nsThread.cpp:633]
NS_ProcessNextEvent_P [nsThreadUtils.cpp:250]
mozilla::ipc::MessagePump::Run [MessagePump.cpp:110]
MessageLoop::RunInternal [message_loop.cc:220]
MessageLoop::RunHandler [message_loop.cc:203]
MessageLoop::Run [message_loop.cc:176]
nsBaseAppShell::Run [nsBaseAppShell.cpp:198]
nsAppStartup::Run [nsAppStartup.cpp:220]
XRE_main [nsAppRunner.cpp:3781]
main [nsBrowserApp.cpp:158]


Also occurs on mozilla-central:
Rev3 Fedora 12 mozilla-central debug test mochitest-other on 2011/03/01 13:11:15
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1299013875.1299017568.10848.gz&fulltext=1

Comment 1

[Jesse Ruderman]

I guess this happens on every run:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1313673486.1313677240.20648.gz&fulltext=1

Comment 2

[Jesse Ruderman]

This one is kind of bad.  I don't want to ignore this assertion, and I don't want to disable testing of accessibility.

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: patch

this seems more correct, but I can't find a case this fixes other than test_1.html doesn't make us assert anymore.  I suspect bug 452584  may or not be the same issue, but the tests in test_1.html that are theoretically disabled because of that bug don't pass with this patch so...

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: patch

first one was only test

Comment 5

[alexander :surkov (:asurkov)]

Trev, can you describe in details why does it happen please?

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> Trev, can you describe in details why does it happen please?

so, the first time through the loop we start startAcc and startFrame, and leave the end things pointing to null.  The next time through we have a HTMLBRAccessible which isn't a text accessible so we don't go through the path that would set endAcc / endFrame because nsAccUtils::IsText() returns false.  Then we break out of the loop because that was all of the range, but endAcc / endFrame are still null so we set them to be the same as start.  (I'm pretty sure that's the way it works, but would need to check tomorrow to for more detail).

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 722625 [details] [diff] [review]
patch

Review of attachment 722625 [details] [diff] [review]:
-----------------------------------------------------------------

I think this code should return wrong aEndOffset in described case but I would rather rewrite this code from scratch than trying to fix it.

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +233,5 @@
>    else if (startOffset > endOffset) {
>      return nullptr;
>    }
>  
> +  nsIFrame *startFrame = nullptr, * endFrame = nullptr; if (aEndFrame) {

nit: nsIFrame* name

if (aEndFrame) must be on own line

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> Comment on attachment 722625 [details] [diff] [review]
> patch
> 
> Review of attachment 722625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this code should return wrong aEndOffset in described case but I
> would rather rewrite this code from scratch than trying to fix it.
> 
> ::: accessible/src/generic/HyperTextAccessible.cpp
> @@ +233,5 @@
> >    else if (startOffset > endOffset) {
> >      return nullptr;
> >    }
> >  
> > +  nsIFrame *startFrame = nullptr, * endFrame = nullptr; if (aEndFrame) {
so do we want to check this in?

> nit: nsIFrame* name
> 
> if (aEndFrame) must be on own line

yeah, not sure how that happened..

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> > I think this code should return wrong aEndOffset in described case but I
> > would rather rewrite this code from scratch than trying to fix it.
> > 

> so do we want to check this in?

I think so, no known regressions, it fixes assertion. I plan to touch this code soon anyway.

> > > +  nsIFrame *startFrame = nullptr, * endFrame = nullptr; if (aEndFrame) {

btw, pls, keep startFrame and endFrame definitions on separate lines

Comment 10

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/397034d19dfc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Follow-up fix to editabletext/test_1.html annotations.
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b8e0a0bdb7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/397034d19dfc
https://hg.mozilla.org/mozilla-central/rev/79b8e0a0bdb7