"ASSERTION: Invalid offset" with late-enabled accessibility

RESOLVED FIXED in mozilla10

Status

()

Core
Disability Access APIs
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: davidb)

Tracking

({assertion, testcase})

Trunk
mozilla10
x86
Mac OS X
assertion, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 554083 [details]
testcase (see comment 0)

1. Install https://www.squarefree.com/extensions/domFuzzLite2.xpi
2. Load the testcase from the command line.
3. Click the button.
4. Wait patiently for 10 seconds, not moving focus away from the browser.

Result: a pile of 

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

(Related to bug 637898?)
(Reporter)

Comment 1

7 years ago
Created attachment 554085 [details]
output, including stacks for assertions leading up to "Invalid offset"
We don't appear to be doing anything special here, just calling GetRenderedText() with a string and otherwise default arguments on a frame.  Is there times we shouldn't be calling GetRenderedText()?    Particularly during a refresh observers WillRefresh()
Are you flushing layout before calling GetRenderedText? You need to.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Are you flushing layout before calling GetRenderedText? You need to.

David, this should be the the case since its  called from the refresh observer right?
The frame tree should be safe to use off the refresh driver but when it comes to rendered text accuracy I'm not so sure... Robert is probably suggesting the flush because we need to :)
I don't think the refresh driver will necessarily flush before calling WillRefresh (it could be a performance hit to do that automatically). You should flush before your first call to GetRenderedText. But if your WillRefresh callback doesn't need to call GetRenderedText, don't flush.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> I don't think the refresh driver will necessarily flush before calling
> WillRefresh (it could be a performance hit to do that automatically). You
> should flush before your first call to GetRenderedText. But if your
> WillRefresh callback doesn't need to call GetRenderedText, don't flush.

ok, you mena flush with nsIPrsShell::FlushPendingNotifications() right?  What flush type do we want here? is Flush_Style enough, or do we want Flush_Layout?
Attachment #554708 - Flags: review?(surkov.alexander)
Attachment #554708 - Flags: review?(bolterbugz)
roc: is this a potential security problem or can it simply lead to bad layout?

Comment 12

7 years ago
A11y adds listener for Flush_Display, so when we are called then there's no guarantee that layout is done? It appears I don't get idea of refresh observer.

Comment 13

7 years ago
Comment on attachment 554708 [details] [diff] [review]
patch

even if this is correct approach (what is surprise for me) but wrong place anyway, layout flush may be not cheap call and making it for every created accessible doesn't look reasonable.
Attachment #554708 - Flags: review?(surkov.alexander)
Attachment #554708 - Flags: review?(bolterbugz)
Attachment #554708 - Flags: review-
> so when we are called then there's no guarantee that layout is done?

Not given interruptible reflow, right?  If you turn off reflow interruption, does this bug disappear?
(In reply to Boris Zbarsky (:bz) from comment #14)
> > so when we are called then there's no guarantee that layout is done?
> 
> Not given interruptible reflow, right?  If you turn off reflow interruption,
> does this bug disappear?

This sounds like it should work. How do we turn it off? Is this a setting we can change when accessibility is instantiated?(In reply to alexander surkov from comment #13)

> Comment on attachment 554708 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> even if this is correct approach (what is surprise for me) but wrong place
> anyway, layout flush may be not cheap call and making it for every created
> accessible doesn't look reasonable.

It should be cheap most of the time right?

It isn't clear to me which approach is better but it sounds like we need to do one of them.
Assignee: nobody → bolterbugz

Comment 16

7 years ago
(In reply to David Bolter [:davidb] from comment #15)

> > Comment on attachment 554708 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch
> > 
> > even if this is correct approach (what is surprise for me) but wrong place
> > anyway, layout flush may be not cheap call and making it for every created
> > accessible doesn't look reasonable.
> 
> It should be cheap most of the time right?
> 
> It isn't clear to me which approach is better but it sounds like we need to
> do one of them.

if what we see is an interruptible reflow then this approach is wrong because it gets rid the idea of interruptible reflow
Okay if we need the perf gains of incorruptible reflow let's go with the flush. Sound good? I'd say take Trevor's patch.

Comment 18

7 years ago
(In reply to David Bolter [:davidb] from comment #17)
> Okay if we need the perf gains of incorruptible reflow let's go with the
> flush.

I don't understand how you're going to get perf gains of interruptible reflow by flush.

> Sound good? I'd say take Trevor's patch.

Did you see my comment #16? Doesn't it make sense?
You're right that in both cases we circumvent interruptible reflow but maybe we do it less in the flush case.

Is there any other way to fix this bug?

Should we pushing that GetRenderedText be doable off the refresh observer? Boris is that possible? (Or crazy talk?)

Comment 20

7 years ago
I'd say we need to ignore refresh driver callback in case of interruptible reflow. In other words do a11y processing when reflow is done.
That may be never, no?

What is GetRenderedText doing that's not safe after a reflow interrupt?  Why is our painting code not getting bitten by the same issues?

Comment 22

7 years ago
(In reply to Boris Zbarsky (:bz) from comment #21)
> That may be never, no?

this is a problem then. on the another hand if we create accessible tree and expose visual information (like rendered text, boundaries) when layout is not done then it's a problem too.

> What is GetRenderedText doing that's not safe after a reflow interrupt?  Why
> is our painting code not getting bitten by the same issues?

good question, I assume painting doesn't do flush, so when does it happen?
With interruptible reflow we paint when layout is not "done".  I don't see why it's being more of a problem for a11y, hence my question from comment 21.

Painting just happens after reflow.  If the reflow was interrupted, we paint the state of the page as it is after that interrupted reflow.
NI can't recreate via STR in comment 0 on Linux trunk.
(In reply to David Bolter [:davidb] from comment #24)
> NI

Also, looking for a nice shrubbery - have you seen one?
Jesse can you see it in trunk?

Comment 27

7 years ago
bug 627647 could be similar issue and have same fix
I can't recreate this assertion on Windows either. Can anyone recreate this on trunk?
(In reply to Boris Zbarsky (:bz) from comment #21)

> What is GetRenderedText doing that's not safe after a reflow interrupt?  Why
> is our painting code not getting bitten by the same issues?

Nothing jumps out as unsafe via code inspection - wish I could recreate it and debug.
Surkov FWIW I agree with your comment 16 (and comment 13).
Oops comment 28 was tested in e10s compat build... rebuilding.
Recreated it.
Created attachment 566858 [details] [diff] [review]
patch 2
Attachment #554708 - Attachment is obsolete: true
Attachment #566858 - Flags: review?(surkov.alexander)

Comment 34

7 years ago
(In reply to David Bolter [:davidb] from comment #33)
> Created attachment 566858 [details] [diff] [review] [diff] [details] [review]
> patch 2

David, patch description, some background what happens here and how it fixes please?

Comment 35

7 years ago
Comment on attachment 566858 [details] [diff] [review]
patch 2

>     // Keep and copy the appropriate chars withing the caller's requested range
>     const nsStyleText* textStyle = textFrame->GetStyleText();
>     while (iter.GetOriginalOffset() < trimmedContentOffsets.GetEnd() &&
>-           keptCharsLength < aSkippedMaxLength) {
>+           keptCharsLength < textFrag->GetLength()) {

I don't think this is correct because keptCharsLength is a length of rendered text and strictly speaking it's not defined by currently traversed text fragment. For me it sounds like comparison of apples with chairs but I may be wrong. In either case it's better to ask Roc for review because I'm not a peer of layout module.
Attachment #566858 - Flags: review?(surkov.alexander)

Comment 36

7 years ago
(In reply to alexander surkov from comment #35)
> Comment on attachment 566858 [details] [diff] [review] [diff] [details] [review]
> patch 2
> 
> >     // Keep and copy the appropriate chars withing the caller's requested range
> >     const nsStyleText* textStyle = textFrame->GetStyleText();
> >     while (iter.GetOriginalOffset() < trimmedContentOffsets.GetEnd() &&
> >-           keptCharsLength < aSkippedMaxLength) {
> >+           keptCharsLength < textFrag->GetLength()) {
> 
> I don't think this is correct because keptCharsLength is a length of
> rendered text and strictly speaking it's not defined by currently traversed
> text fragment. 

ah, you're looking at the whole content text, that makes sense

Comment 37

7 years ago
How does it happen that iter.GetOriginalOffset() < trimmedContentOffsets.GetEnd() is true but you can't move iterator forward, i.e. iter.AdvanceOriginal(1).
Comment on attachment 566858 [details] [diff] [review]
patch 2

Yeah let's get Robert to take a look. Robert note you introduced the assert that this bug hits here:
http://hg.mozilla.org/mozilla-central/diff/3ab5e1ea160a/gfx/thebes/src/gfxSkipChars.cpp

I have the feeling GetRenderedText could be improved beyond this patch but I think I'd need time to learn quite a bit about text layout code.
Attachment #566858 - Flags: review?(roc)
This effectively disables support for aSkippedMaxLength, which I presume is not what you want.
Perhaps GetRenderedText should just stop when it sees a dirty textframe? For such frames, the offsets and TrimmedOffsets are not reliable.

Comment 41

7 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> Perhaps GetRenderedText should just stop when it sees a dirty textframe? For
> such frames, the offsets and TrimmedOffsets are not reliable.

Will ReflowText trigger after frames gets undirty http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#7433 or does dirty frame never gets undirty and we can just ignore it?
Created attachment 567162 [details] [diff] [review]
patch 3

Robert, is this what you had in mind? It seems to make sense to me... but I am curious about the answer Alexander's question above.

(I haven't test this patch beyond confirming the assertion is not hit)
Attachment #566858 - Attachment is obsolete: true
Attachment #566858 - Flags: review?(roc)
Attachment #567162 - Flags: review?(roc)
(In reply to alexander surkov from comment #41)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> > Perhaps GetRenderedText should just stop when it sees a dirty textframe? For
> > such frames, the offsets and TrimmedOffsets are not reliable.
> 
> Will ReflowText trigger after frames gets undirty
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsTextFrameThebes.cpp#7433 or does dirty frame never gets undirty and we can
> just ignore it?

Dirty frames will always be reflowed eventually ... well, unless the page is closed first.

Comment 45

7 years ago
Comment on attachment 567162 [details] [diff] [review]
patch 3


>   // Build skipChars and copy text, for each text frame in this continuation block
>   for (textFrame = this; textFrame;
>        textFrame = static_cast<nsTextFrame*>(textFrame->GetNextContinuation())) {
>     // For each text frame continuation in this block ...
> 
>+    if (textFrame->GetStateBits() & NS_FRAME_IS_DIRTY) {
>+      // We don't trust dirty frames, expecially when computing rendered text.
>+      break;
>+    }

are we sure that next frame to dirty frame is dirty too?

Comment 46

7 years ago
https://hg.mozilla.org/mozilla-central/rev/783daf190579
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(In reply to alexander surkov from comment #45)
> Comment on attachment 567162 [details] [diff] [review] [diff] [details] [review]
> patch 3

> >+    if (textFrame->GetStateBits() & NS_FRAME_IS_DIRTY) {
> >+      // We don't trust dirty frames, expecially when computing rendered text.
> >+      break;
> >+    }
> 
> are we sure that next frame to dirty frame is dirty too?

I'm not sure, but the continuation is presumably going to be reflowed.
Backed on inbound to see if it helps some a11y talos weirdness (see dev-tree-management for details).

https://hg.mozilla.org/integration/mozilla-inbound/rev/26a69ff93c13
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The data tells me this changeset wasn't to blame for the Talos regression so relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a237243d5ca
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Landed in central.
Group: core-security
You need to log in before you can comment on or make changes to this bug.