680085 - "ASSERTION: Invalid offset" with late-enabled accessibility

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jesse Ruderman]

Attachment: 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?)

Comment 1

[Jesse Ruderman]

Attachment: output, including stacks for assertions leading up to "Invalid offset"

Comment 2

[Trevor Saunders (:tbsaunde)]

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

Comment 3

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

Are you flushing layout before calling GetRenderedText? You need to.

Comment 4

[Trevor Saunders (:tbsaunde)]

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

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

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 :)

Comment 6

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

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.

Comment 7

[Trevor Saunders (:tbsaunde)]

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

Comment 8

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

Flush_Layout.

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 10

[Daniel Veditz [:dveditz]]

roc: is this a potential security problem or can it simply lead to bad layout?

Comment 11

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

I don't know.

Comment 12

[alexander :surkov (:asurkov)]

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

[alexander :surkov (:asurkov)]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 16

[alexander :surkov (:asurkov)]

(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

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

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

[alexander :surkov (:asurkov)]

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

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

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

[alexander :surkov (:asurkov)]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

[alexander :surkov (:asurkov)]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

NI can't recreate via STR in comment 0 on Linux trunk.

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to David Bolter [:davidb] from comment #24)
> NI

Also, looking for a nice shrubbery - have you seen one?

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

Jesse can you see it in trunk?

Comment 27

[alexander :surkov (:asurkov)]

bug 627647 could be similar issue and have same fix

Comment 28

[David Bolter [:davidb] (NeedInfo me for attention)]

I can't recreate this assertion on Windows either. Can anyone recreate this on trunk?

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

Surkov FWIW I agree with your comment 16 (and comment 13).

Comment 31

[David Bolter [:davidb] (NeedInfo me for attention)]

Oops comment 28 was tested in e10s compat build... rebuilding.

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

Recreated it.

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2

Comment 34

[alexander :surkov (:asurkov)]

(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

[alexander :surkov (:asurkov)]

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.

Comment 36

[alexander :surkov (:asurkov)]

(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

[alexander :surkov (:asurkov)]

How does it happen that iter.GetOriginalOffset() < trimmedContentOffsets.GetEnd() is true but you can't move iterator forward, i.e. iter.AdvanceOriginal(1).

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 39

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

This effectively disables support for aSkippedMaxLength, which I presume is not what you want.

Comment 40

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

Perhaps GetRenderedText should just stop when it sees a dirty textframe? For such frames, the offsets and TrimmedOffsets are not reliable.

Comment 41

[alexander :surkov (:asurkov)]

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

Comment 42

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: 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)

Comment 43

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

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

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/783daf190579

Comment 45

[alexander :surkov (:asurkov)]

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

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/mozilla-central/rev/783daf190579

Comment 47

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 48

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 49

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 50

[David Bolter [:davidb] (NeedInfo me for attention)]

Landed in central.