Last Comment Bug 634197 - assert if accessible tree update triggers reflow
: assert if accessible tree update triggers reflow
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: alexander :surkov
:
Mentors:
Depends on: 692608
Blocks: treeupdatea11y 629912
  Show dependency treegraph
 
Reported: 2011-02-15 00:36 PST by alexander :surkov
Modified: 2011-10-06 14:52 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.64 KB, patch)
2011-10-04 01:31 PDT, alexander :surkov
dbolter: review+
Details | Diff | Splinter Review
patch2 (4.67 KB, patch)
2011-10-05 07:19 PDT, alexander :surkov
bzbarsky: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-02-15 00:36:27 PST
spun off the bug 629912 comment #36
Comment 1 alexander :surkov 2011-02-15 00:37:15 PST
Boris, do I get right one of such bad things is bug 630486?
Comment 2 David Bolter [:davidb] 2011-02-15 06:24:05 PST
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.
Comment 3 alexander :surkov 2011-02-15 10:02:48 PST
Boris, is it ok to call nsIFrame::GetRenderedText (we do that when refresh driver calls into us and we update the tree)?
Comment 4 Boris Zbarsky [:bz] 2011-02-15 10:35:17 PST
> 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.
Comment 5 Boris Zbarsky [:bz] 2011-02-15 10:35:39 PST
> Boris, is it ok to call nsIFrame::GetRenderedText

Yes.
Comment 6 alexander :surkov 2011-02-15 10:46:17 PST
(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).
Comment 7 Boris Zbarsky [:bz] 2011-02-15 10:52:41 PST
I'm not sure what comment 6 is asking.
Comment 8 alexander :surkov 2011-02-15 11:10:46 PST
(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
Comment 9 Boris Zbarsky [:bz] 2011-02-15 11:25:13 PST
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.
Comment 10 David Bolter [:davidb] 2011-02-17 06:09:48 PST
I wonder if there is a way instrument against causing a flush at the wrong time using static analysis and source annotation?
Comment 11 David Bolter [:davidb] 2011-02-17 12:21:02 PST
Alexander I'm moving this to blocking .x pending further investigation. Please renominate with rationale if you disagree.
Comment 12 David Bolter [:davidb] 2011-02-17 12:23:19 PST
Note I felt we didn't need to hard block anymore given Bz's recent comment: bug 634200 comment 2
Comment 13 alexander :surkov 2011-02-17 21:20:40 PST
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?
Comment 14 Boris Zbarsky [:bz] 2011-02-17 21:39:37 PST
You want to assert that you're not entering PresShell::FlushPendingNotifications while you're inside your own WillRefresh callback?
Comment 15 alexander :surkov 2011-02-17 21:44:58 PST
(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.
Comment 16 Boris Zbarsky [:bz] 2011-02-17 21:48:29 PST
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....
Comment 17 alexander :surkov 2011-10-04 01:31:44 PDT
Created attachment 564482 [details] [diff] [review]
patch
Comment 18 Marco Zehe (:MarcoZ) 2011-10-04 01:47:06 PDT
Comment on attachment 564482 [details] [diff] [review]
patch

This is just debugging code, right?
Comment 19 alexander :surkov 2011-10-04 02:18:44 PDT
yes
Comment 20 David Bolter [:davidb] 2011-10-04 06:30:12 PDT
Comment on attachment 564482 [details] [diff] [review]
patch

Do the asserts hit?
Comment 21 Boris Zbarsky [:bz] 2011-10-04 10:30:07 PDT
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?
Comment 22 alexander :surkov 2011-10-04 18:58:42 PDT
(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?
Comment 23 Boris Zbarsky [:bz] 2011-10-04 20:02:11 PDT
Is it a layout refresh, or a refresh of some a11y data structures that's happening?
Comment 24 alexander :surkov 2011-10-04 20:04:05 PDT
(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
Comment 25 Boris Zbarsky [:bz] 2011-10-04 20:06:49 PDT
OK, then LayoutRefresh doesn't sound right....

Maybe just IsProcessingRefreshDriverNotification() if that's what it really means?
Comment 26 alexander :surkov 2011-10-04 20:12:28 PDT
sounds good, thank you. a little bit long though
Comment 27 alexander :surkov 2011-10-05 07:19:40 PDT
Created attachment 564839 [details] [diff] [review]
patch2
Comment 28 Boris Zbarsky [:bz] 2011-10-05 09:51:00 PDT
Comment on attachment 564839 [details] [diff] [review]
patch2

r=me
Comment 29 alexander :surkov 2011-10-05 19:15:49 PDT
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/00dcb64cb920
Comment 30 Ed Morley [:emorley] 2011-10-06 08:38:44 PDT
https://hg.mozilla.org/mozilla-central/rev/00dcb64cb920

Note You need to log in before you can comment on or make changes to this bug.