987023 - "ASSERTION: Invalid offset" in gfxSkipChars with editor, accessibility, CSS grid

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Set:
     user_pref("layout.css.grid.enabled", true);
     user_pref("devtools.chrome.enabled", true);
 
2. Enable accessibility by pasting the following into the Browser Console:

Components.classes["@mozilla.org/accessibilityService;1"].getService(Components.interfaces.nsIAccessibleRetrieval);

3. Load the testcase.

4. Click in the content area (??)

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

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

Comment 3

[Simon Sapin (:SimonSapin)]

I’m not disputing that this is a security issue, but I’m curious what makes it so?

So far, 'display: grid' and 'display: inline-grid' create a nsGridContainerFrame that does almost nothing besides inheriting from nsContainerFrame. It might be that something in this stack requires all frame types to implement (override) some method that is not implemented yet.

Comment 4

[Simon Sapin (:SimonSapin)]

For what it’s worth, I couldn’t reproduce the same assertion. I’m getting this one instead:

[20343] ###!!! ASSERTION: Can only call this on frames that have been reflowed: '!(GetStateBits() & NS_FRAME_FIRST_REFLOW) || (GetParent()->GetStateBits() & NS_FRAME_TOO_DEEP_IN_FRAME_TREE)', file /home/simon/projects/gecko-dev/layout/generic/nsTextFrame.cpp, line 2645

But it seems to be a non-fatal one. gdb does not give me a prompt, so I don’t know how to get a stack trace for this assertion.

Comment 5

[Trevor Saunders (:tbsaunde)]

> But it seems to be a non-fatal one. gdb does not give me a prompt, so I
> don’t know how to get a stack trace for this assertion.

the easiest way I know of is setting XPCOM_DEBUG_BREAK=trap in the environment.

Comment 6

[Jesse Ruderman]

> I’m not disputing that this is a security issue, but I’m curious what makes it so?

I marked this bug as security-sensitive because there have been lots of bugs where the gfxSkipChars "invalid offset" assertion was followed by scarier things.

Comment 7

[Jesse Ruderman]

> For what it’s worth, I couldn’t reproduce the same assertion. I’m getting this one instead:

That's what I get if I skip step 2.

Comment 8

[Daniel Veditz [:dveditz]]

Simon: were you able to reproduce the first assertion if you didn't skip step 2?

Comment 9

[(no longer active)]

This looks Accessibility related; even though the code uses contenteditable, the stack is an accessibility event running.

Comment 10

[Simon Sapin (:SimonSapin)]

(In reply to Daniel Veditz [:dveditz] from comment #8)
> Simon: were you able to reproduce the first assertion if you didn't skip
> step 2?

I was, but unfortunately I don’t understand either assertion and don’t know how to debug them.

Comment 11

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

I'll get this looked at.

Comment 12

[Trevor Saunders (:tbsaunde)]

Something is clearly wrong with HyperTextAccessible::CaretOffset() given that it needs to be callable in this context even if we don't do it.  It may be that bug 949518 will magically fix this, I don't really understand what's happening there.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> This looks Accessibility related; even though the code uses contenteditable,
> the stack is an accessibility event running.

the code is running on WillRefresh of refresh driver call. I supposed that WillRefresh is called at "good" times only. It's not correct?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> Something is clearly wrong with HyperTextAccessible::CaretOffset() 

can you give details please?

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > Something is clearly wrong with HyperTextAccessible::CaretOffset() 
> 
> can you give details please?

I said everything I know.

Comment 16

[Daniel Holbert [:dholbert]]

So nsGridContainerFrame ends up inheriting nsFrame::Reflow, which sets its outparameters and returns without ever inspecting the children.

So, the nsGridContainerFrame's child (a text frame) never gets reflowed. I'm betting that's what the problem is here.

Perhaps our stub nsGridContainerFrame impl needs its own slightly-smarter stub reflow implementation, which reflows all of its children at position 0,0 with unconstrained available height. (Or maybe we should put a reflow impl like that on nsContainerFrame, for future stub container classes to inherit, to prevent them from hitting variants of this bug.)

Comment 17

[Daniel Holbert [:dholbert]]

I suspect this is really a layout bug, per comment 16.

CC'ing mats, who's starting to work on some grid stuff. I suspect we'll be needing an improved (if incomplete) reflow method soon, to test the beginnings of our grid impl.  Whenever we add that, I suspect it'll fix this bug, as long as it gives the children a reflow.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's a reduced testcase that still reproduces this for me (using STR from comment 0).

No scripting or contenteditable required.

Comment 19

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

Daniel can you or Mats own this one?

Comment 20

[Mats Palmgren (inactive)]

(In reply to Jesse Ruderman from comment #0)
> 2. Enable accessibility by pasting the following into the Browser Console:
> 
> Components.classes["@mozilla.org/accessibilityService;1"].
> getService(Components.interfaces.nsIAccessibleRetrieval);

I get an error "TypeError: Components.classes is undefined"
when I do that.

Is there an alternative way of enabling a11y?

Comment 21

[alexander :surkov (:asurkov)]

I do use DOM Inspector (accessibility tree). Also you can run platform tools (like Event Watcher on windows) or Accessibility Inspector on Mac.

Comment 22

[Mats Palmgren (inactive)]

Thanks, I can reproduce it locally with that.  The bug does not occur in my
local css-grid tree that has reflow for the grid items so it will be fixed
when those patches lands.

I'm marking this bug sec-other since the bug only occurs if you enable
a pref in about:config (layout.css.grid.enabled).

Comment 23

[Mats Palmgren (inactive)]

Fwiw, I think it's very unfortunate that this file uses gfxSkipChars
in the first place.
http://mxr.mozilla.org/mozilla-central/source/accessible/src/generic/HyperTextAccessible.cpp#1721

Does it really need to do that?  Can you use GeometryUtils on the DOM nodes
instead?  If not, please specify what you need so we can add the missing
functionality so you can work on nsIContent instead of nsIFrame.
http://dev.w3.org/csswg/cssom-view/#the-geometryutils-interface

Comment 24

[alexander :surkov (:asurkov)]

this function converts content offset to rendered offset (DOM point to frame point). I don't really have ideas how GeometryUtils can do the same.

Comment 25

[Mats Palmgren (inactive)]

Yes, but my point is that RenderedToContentOffset shouldn't need to exist
if we had DOM APIs, or at least internal nsIContent methods to give you
the information that you need.  So on a high level, what is the information
that you need?  I assumed it was the geometry of the rendered DOM nodes,
or some Range of them?  What else?

Comment 26

[alexander :surkov (:asurkov)]

say you have
<p>hello     hello</p>
which is rendered as
<p>hello hello</p>
if you have DOM point (textnode, 10) - a point before 'h' of second 'hello' then we need to convert it into rendered offset, ie. (text node, 6) - a point right before rendered 'h' of second 'hello'.

Comment 27

[Mats Palmgren (inactive)]

Yes, but the rendered offset is kind of useless except in other internal
nsIFrame methods, so you shouldn't need it if you had a proper content
API to give you the information you're looking for.

Let's ignore the implementation details for now -- can you describe the
high level thing(s) that this code needs to accomplish?

Comment 28

[alexander :surkov (:asurkov)]

screen readers work with rendered offsets, DOM operates with content offsets. If all DOM stuffs like selection provided an option to work with rendered offsets then that'd be what we need.

Comment 29

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #28)
> screen readers work with rendered offsets, DOM operates with content

that's kind of a meaningless statement since its just an implementation detail that the offsets exposed to accessibility APIs have anything to do with offsets in frames.

(In reply to Mats Palmgren (:mats) from comment #27)
> Yes, but the rendered offset is kind of useless except in other internal
> nsIFrame methods, so you shouldn't need it if you had a proper content
> API to give you the information you're looking for.

The issues with using just content stuff that I can think of are
- text from css generated content and list bullets
- line / word / navigation is tied in with nsSelection and that's sort of tied into frames.

> Let's ignore the implementation details for now -- can you describe the
> high level thing(s) that this code needs to accomplish?

The basic stuff is grab all the text under a DOM node, break it up by words / lines / characters, then you need to be able to provide bounds for each of those objects.  You also need to be able to select / unselect chunks of text, but that's mostly deligated to selection code.  basically you need to implement accessible/public/nsIAccessible{Hyper,Editable,}text.idl for every DOM node.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #29)
> The basic stuff is grab all the text under a DOM node, break it up by words
> / lines / characters, 

that's wrong statement in general since AT needs to grab rendered text and that's the whole point.

Comment 31

[Mats Palmgren (inactive)]

I filed bug 1016574, let's continue the discussion of what is needed there.

Comment 32

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #30)
> (In reply to Trevor Saunders (:tbsaunde) from comment #29)
> > The basic stuff is grab all the text under a DOM node, break it up by words
> > / lines / characters, 
> 
> that's wrong statement in general since AT needs to grab rendered text and
> that's the whole point.

other than arguing about terms I'm not sure what your point is

Comment 33

[Mats Palmgren (inactive)]

The initial batch of CSS Grid layout patches are now in Nightly, so this bug is fixed.
I don't see any assertions when I try the STR in a local debug build.

Jesse, please confirm if you can.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Adding bkerensa since he wasn't able to see this and needs to for ESR.