Closed Bug 634197 Opened 9 years ago Closed 8 years ago

assert if accessible tree update triggers reflow

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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]
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.
(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
(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.
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
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?
(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....
Attached patch patch (obsolete) — Splinter Review
Attachment #564482 - Flags: review?(bzbarsky)
Attachment #564482 - Flags: review?(bolterbugz)
Comment on attachment 564482 [details] [diff] [review]
patch

This is just debugging code, right?
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?
(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?
(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?
sounds good, thank you. a little bit long though
Attached patch patch2Splinter Review
Attachment #564482 - Attachment is obsolete: true
Attachment #564839 - Flags: review?(bzbarsky)
Attachment #564482 - Flags: review?(bzbarsky)
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+
https://hg.mozilla.org/mozilla-central/rev/00dcb64cb920
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Depends on: 692608
You need to log in before you can comment on or make changes to this bug.