assert if accessible tree update triggers reflow

RESOLVED FIXED in mozilla10

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla10
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
spun off the bug 629912 comment #36
(Assignee)

Comment 1

6 years ago
Boris, do I get right one of such bad things is bug 630486?
OK to paste a bit of that:

We have to be careful:
1)  Any time you flush.
2)  Any time you call code you don't control and that might flush.

Examples of #2 would be any DOM API asking for position information, say.  Or
scrolling.  Or anything that spins the event loop, of course."

(I don't have access to bug 630486)

I agree this should block until we figure things out here.
blocking2.0: ? → final+
Whiteboard: [hardblocker]
(Assignee)

Comment 3

6 years ago
Boris, is it ok to call nsIFrame::GetRenderedText (we do that when refresh driver calls into us and we update the tree)?
> Boris, do I get right one of such bad things is bug 630486?

That one doesn't look bad, as long as the a11y code on the stack can deal with arbitrary script running.

The refresh driver expects that arbitrary script might run under the WillRefresh call it makes, so that part is fine.
> Boris, is it ok to call nsIFrame::GetRenderedText

Yes.
(Assignee)

Comment 6

6 years ago
(In reply to comment #4)
> > Boris, do I get right one of such bad things is bug 630486?
> 
> That one doesn't look bad, as long as the a11y code on the stack can deal with
> arbitrary script running.
> 
> The refresh driver expects that arbitrary script might run under the
> WillRefresh call it makes, so that part is fine.

It doesn't trigger text reflow? It always happen after timeout? (when we create accessible tree we and run popup children creation).
I'm not sure what comment 6 is asking.
Status: ASSIGNED → NEW
(Assignee)

Comment 8

6 years ago
(In reply to comment #7)
> I'm not sure what comment 6 is asking.

I'm trying figure out if "Any time you call code you don't control and that might flush." possible. Is the following scenario valid?

1) refresh driver calls into us
2) we create an accessible tree
3) trigger popup frame children creation
4) getting text reflow
That's fine from the refresh driver's pov if you're doing it under WillRefresh.

So as long as _your_ code is ok with that, it's fine.

Updated

6 years ago
Blocks: 629912

Updated

6 years ago
Whiteboard: [hardblocker] → [hardblocker] see possible dupe bug 629912
I wonder if there is a way instrument against causing a flush at the wrong time using static analysis and source annotation?
Alexander I'm moving this to blocking .x pending further investigation. Please renominate with rationale if you disagree.
blocking2.0: final+ → .x
Whiteboard: [hardblocker] see possible dupe bug 629912
Note I felt we didn't need to hard block anymore given Bz's recent comment: bug 634200 comment 2
(Assignee)

Comment 13

6 years ago
We don't have other explicit layout flush in a11y. We could close this bug as fixed but it'd be nice to assert if something flush (or reflows) while we're inside WillRefresh. Boris, is there a way to add an assertion for that?
blocking2.0: .x → ---
You want to assert that you're not entering PresShell::FlushPendingNotifications while you're inside your own WillRefresh callback?
(Assignee)

Comment 15

6 years ago
(In reply to comment #14)
> You want to assert that you're not entering
> PresShell::FlushPendingNotifications while you're inside your own WillRefresh
> callback?

That'd be great. I don't know if reflow is different from FlushPendingNotifications, i.e. can be triggered by something else.
It can, but only if documents are resized.

I suppose you could just add some state on the accessibility service or something and assert it in presshell....
(Assignee)

Updated

6 years ago
Blocks: 572951
(Assignee)

Comment 17

6 years ago
Created attachment 564482 [details] [diff] [review]
patch
Attachment #564482 - Flags: review?(bzbarsky)
Attachment #564482 - Flags: review?(bolterbugz)
Comment on attachment 564482 [details] [diff] [review]
patch

This is just debugging code, right?
(Assignee)

Comment 19

6 years ago
yes
Comment on attachment 564482 [details] [diff] [review]
patch

Do the asserts hit?
Attachment #564482 - Flags: review?(bolterbugz) → review+
Comment on attachment 564482 [details] [diff] [review]
patch

IsLayoutRefreshProcessed is a really odd name for this function, since it implies that something has _already_ happened.  What does it actually mean for this function to return true?  It seems to mean that something _is_ happening, right?
(Assignee)

Comment 22

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #21)
> Comment on attachment 564482 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> IsLayoutRefreshProcessed is a really odd name for this function, since it
> implies that something has _already_ happened.  What does it actually mean
> for this function to return true?  It seems to mean that something _is_
> happening, right?

right, thank you, oversight. IsLayoutRefreshProcessing is better?
Is it a layout refresh, or a refresh of some a11y data structures that's happening?
(Assignee)

Comment 24

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #23)
> Is it a layout refresh, or a refresh of some a11y data structures that's
> happening?

refresh of a11y
OK, then LayoutRefresh doesn't sound right....

Maybe just IsProcessingRefreshDriverNotification() if that's what it really means?
(Assignee)

Comment 26

6 years ago
sounds good, thank you. a little bit long though
(Assignee)

Comment 27

6 years ago
Created attachment 564839 [details] [diff] [review]
patch2
Attachment #564482 - Attachment is obsolete: true
Attachment #564839 - Flags: review?(bzbarsky)
Attachment #564482 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Summary: make sure accessible tree creation doesn't trigger reflow → assert if accessible tree update triggers reflow
Comment on attachment 564839 [details] [diff] [review]
patch2

r=me
Attachment #564839 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 29

6 years ago
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/00dcb64cb920
https://hg.mozilla.org/mozilla-central/rev/00dcb64cb920
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Updated

6 years ago
Depends on: 692608
You need to log in before you can comment on or make changes to this bug.