[Tracking Requested - why for this release]: [Tracking Requested - why for this release]: [Tracking Requested - why for this release]: Huge performance problem +++ This bug was initially created as a clone of Bug #1321892 +++ Build Identifier: https://hg.mozilla.org/mozilla-central/rev/f65ad27efe839ce9df0283840a1a40b4bbc9ead0 Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161202030204 Reproducible: always Steps to reproduce: 1. Enable e10s and activate accessibility(ATOK2016 IME in mycase) 2. Open attachment 8816579 [details] 3. While page is loading, turn mouse wheel for 2-4 seconds. Actual Results: Browser *immediately* hangs up. Windows *becomes* almost unusable. You must kill Firefox.exe Expected Results: Browser should not hang up. Do not make Windows unusable. Setting accessibility.force_disabled=1 seems to fix the hang etc.

Is there a regression range for this

Tracy, could you help take a run at reproducing this?

At least, regression about Firefox hang is as follows: Regression w/ force enable e10s: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=11c3335d77fb751128c456daa204385d69678484&tochange=4de1094b41b27db10a5b0e5683ba4272c505ee9c Regressed by: Bug 1255968 And the hangup is not to reproduce if set accessibility.force_disabled=1 on the bad build.

It was difficult because Browser hangs up, but I got a gecko profiler log.(I am not sure this is correct or not) https://cleopatra.io/#report=0563075ff26a095210ef90a48b1620d7eefa7c91&selection=0,1,2,3,4,5,6,7,8,9,27,30,279,280,58,59,281,282,283,284,285,286,287,288,289,290,291,292,293,294,295,296,297,298,299,300,301,435,436,437,34,35

Marco noted that he also sees a freeze when loading this page with NVDA, so I did some investigation. Findings: f1. Beneath the pre element, there are 37003 accessible children. f2. If you disable virtual buffers and just open the page, there is no freeze with e10s enabled. You can interact with the document (e.g. tab through it) almost immediately and accessibility works as expected. f3. Loading a buffer with e10s enabled takes around 27 seconds on my machine. f4. Loading a buffer with e10s disabled takes around 4 to 5 seconds on my machine. Deductions: d1. This is not caused by simply having accessibility enabled (f2). d2. This suggests the ATOK2016 IME is walking all accessible children (f2, f3, f4). d3. For NVDA, e10s makes this a lot worse (f3, f4). This is because NVDA renders in-process with e10s disabled, but everything is cross-process with e10s enabled. Questions: q1. For the ATOK2016 IME, does this problem occur with a11y enabled but e10s disabled? q2. If so, is it any worse with e10s enabled than with e10s disabled? q3. If not, this is not an e10s specific issue and is probably simply because the IME is fetching a lot of children. q4. Can we get a stack dump during the freeze to confirm whether ATOK2016 is indeed fetching children as deduced in d2? Regarding NVDA, this will probably get better with the COM smart proxy stuff. However, one thing worth noting is that (according to discussions with Aaron) caching children with COM smart proxies isn't currently planned, which may still be a pain point.

Alice0775, are you able to answer these two questions: > q1. For the ATOK2016 IME, does this problem occur with a11y enabled but e10s disabled? > q2. If so, is it any worse with e10s enabled than with e10s disabled?

(In reply to James Teh [:Jamie] from comment #6) > Alice0775, are you able to answer these two questions: > > > q1. For the ATOK2016 IME, does this problem occur with a11y enabled but e10s disabled? It is depended on size of html, with 50% reduced size(attachment 8817303 [details]): *The page loading is almost instant even if turn mouse wheel while page is loading. with original size(attachment 8816579 [details]): *It takes for approx 2 minutes. and then browser becomes normal. > > q2. If so, is it any worse with e10s enabled than with e10s disabled? with 50% reduced size(attachment 8817303 [details]) and original size(attachment 8816579 [details]): *Browser hangs up if turn mouse wheel while page is loading. *The page loading is almost instant if I do not turn mouse wheel.

tracking e10s/a11y issue for 52+

Track 51+ as performance issue related to e10s/a11y.

Hi :ting, Per comment #3, can you help take a look at this one?

I tried to reproduce the issue on Windows 10 by enabling the system's screen reader Narrator, and I used WPR to record a profile. (I don't know how different it is from ATOK2016 IME.) It shows in parent process, the main thread is busy in nsAppShell::ProcessNextNativeEvent() for PeekMessageW(), which goes to oleacc.dll!AccWrap_Annotate::get_accState(), oleacc.dll!AccWrap_Annotate::get_accParent(), and oleacc.dll!AccWrap_Base::Next(). The child process is busy in 1) SyncRunnable::WaitUntilComplete() from MainThreadInvoker::Invoke(), and 2) nsRefreshDriver::Tick() for a11y::NotificationController::WillRefresh(). I don't really understand how does a11y work for why interrutping the reflow in child process (bug 1255968) make the situation worse (comment 9). Any ideas?

We need to figure this bug out, but not for FF 51 (since we aren't supporting this configuration until at least 52+). Adjusting tracking flags accordingly.

I am not sure is this also happened with ATOK2016 IME, but DocAccessibleParent::RecvShowEvent() is called many times with aData.Idx() 0 if I scroll while loading when Narrator is enabled, which then go to ::NotifyWinEvent(). This seems is the reason why parent process main thread is busy in PeekMessageW(). The stack that child process send show event: xul.dll!mozilla::a11y::DocAccessibleChildBase::ShowEvent(mozilla::a11y::AccShowEvent * aShowEvent) Line 92 C++ xul.dll!mozilla::a11y::Accessible::HandleAccEvent(mozilla::a11y::AccEvent * aEvent) Line 845 C++ xul.dll!mozilla::a11y::AccessibleWrap::HandleAccEvent(mozilla::a11y::AccEvent * aEvent) Line 1258 C++ xul.dll!mozilla::a11y::HyperTextAccessibleWrap::HandleAccEvent(mozilla::a11y::AccEvent * aEvent) Line 66 C++ xul.dll!nsEventShell::FireEvent(mozilla::a11y::AccEvent * aEvent) Line 46 C++ xul.dll!mozilla::a11y::NotificationController::ProcessMutationEvents() Line 546 C++ xul.dll!mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp aTime) Line 808 C++ xul.dll!nsRefreshDriver::Tick(__int64 aNowEpoch, mozilla::TimeStamp aNowTime) Line 1805 C++ xul.dll!mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver * driver, __int64 jsnow, mozilla::TimeStamp now) Line 327 C++ xul.dll!mozilla::RefreshDriverTimer::TickRefreshDrivers(__int64 aJsNow, mozilla::TimeStamp aNow, nsTArray<RefPtr<nsRefreshDriver> > & aDrivers) Line 297 C++ xul.dll!mozilla::RefreshDriverTimer::Tick(__int64 jsnow, mozilla::TimeStamp now) Line 319 C++ xul.dll!mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp aTimeStamp) Line 670 C++ xul.dll!mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp aVsyncTimestamp) Line 591 C++ xul.dll!mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp aVsyncTimestamp) Line 511 C++ xul.dll!mozilla::layout::VsyncChild::RecvNotify(const mozilla::TimeStamp & aVsyncTimestamp) Line 66 C++ xul.dll!mozilla::layout::PVsyncChild::OnMessageReceived(const IPC::Message & msg__) Line 170 C++ xul.dll!mozilla::ipc::PBackgroundChild::OnMessageReceived(const IPC::Message & msg__) Line 1467 C++ xul.dll!mozilla::ipc::MessageChannel::DispatchAsyncMessage(const IPC::Message & aMsg) Line 1751 C++ xul.dll!mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message && aMsg) Line 1691 C++ xul.dll!mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask & aTask) Line 1572 C++ xul.dll!mozilla::ipc::MessageChannel::MessageTask::Run() Line 1598 C++ xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1213 C++ xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 381 C++ xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 96 C++ xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate * aDelegate) Line 301 C++ xul.dll!MessageLoop::RunHandler() Line 226 C++ xul.dll!MessageLoop::Run() Line 206 C++ xul.dll!nsBaseAppShell::Run() Line 158 C++ xul.dll!nsAppShell::Run() Line 264 C++ xul.dll!XRE_RunAppShell() Line 924 C++ xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate * aDelegate) Line 278 C++ xul.dll!MessageLoop::RunHandler() Line 226 C++ xul.dll!MessageLoop::Run() Line 206 C++ xul.dll!XRE_InitChildProcess(int aArgc, char * * aArgv, const XREChildData * aChildData) Line 760 C++ firefox.exe!content_process_main(int argc, char * * argv) Line 115 C++

(In reply to Ting-Yu Chou [:ting] from comment #15) > I am not sure is this also happened with ATOK2016 IME, but > DocAccessibleParent::RecvShowEvent() is called many times with aData.Idx() 0 > if I scroll while loading when Narrator is enabled, which then go to > ::NotifyWinEvent(). This seems is the reason why parent process main thread > is busy in PeekMessageW(). it's worth exploring bug 1301829 and bug 1285862, they may be quite helpful here.

(In reply to alexander :surkov from comment #16) > it's worth exploring bug 1301829 and bug 1285862, they may be quite helpful > here. Thanks for the information. Alice0775, since I don't have ATOK2016 IME, could you please try and see if you can reproduce by either NVDA or Narrator? I'd like to make sure I am checking the same issue that you found.

(In reply to Ting-Yu Chou [:ting] from comment #17) > (In reply to alexander :surkov from comment #16) > > it's worth exploring bug 1301829 and bug 1285862, they may be quite helpful > > here. > > Thanks for the information. > > Alice0775, since I don't have ATOK2016 IME, could you please try and see if > you can reproduce by either NVDA or Narrator? I'd like to make sure I am > checking the same issue that you found. I can reproduce the hang on 53.0a3 if Windows Narrator is enabled.

s/53.0a3/53.0a1/

(In reply to Ting-Yu Chou [:ting] from comment #15) > DocAccessibleParent::RecvShowEvent() is called many times with aData.Idx() 0 > if I scroll while loading when Narrator is enabled, which then go to The number of this kind of event is much lesser if PuppetWidget::HasPendingInputEvent() always return false (the behavior before bug 1255968), this seems to be the reason why enabling e10s makes it worse.

(In reply to Aaron Klotz [:aklotz] from comment #2) > Tracy, could you help take a run at reproducing this? Yes, this is reproducible at Alice's test page/attachment with Narrator running on Windows 10 with latest Nightly builds.

So the test html has 17500 of <a> tags in the body: <pre> <a href=20161201-myfile-test-1-klosfdjlfksdjflskdfsdfsdf.csv>20161201-myfile-test-1-kljfsdklfjskldjfsldjfsdlkfskldfsdfsdfdsfd.csv</a> 2016-12-01-04 05:09 3.0M ... </pre> When interruptible reflow (bug 1255968) is disabled, there's only one show event for each <a> tag and its child text node. But when interruptible reflow is enabled, there could be 2 show events, one for <a> tag's accessible and one for its child text node's accessible. The sequences of sending 2 show events when interruptible reflow is enabled: DoReflow() ScheduleTextUpdate() ... reflow is interrupted WillRefresh() ProcessContentInserted() CreateAccessible() // for <a> tag CreateAccessible() // for <a> tag's child text node, it returns nullptr because the text frame has NS_FRAME_IS_DIRTY set AfterInsertion() QueueMutationEvent() // the show event for the accessible of <a> tag ... DoReflow() ScheduleTextUpdate() ... WillRefresh() LookupOrAdd() // add text node to the array for its parent's (<a> tag) accessible ProcessContentInserted() CreateAccessible() // for <a> tag's child text node AfterInsertion() QueueMutationEvent() // the show event for the child text node of <a> tag, note the idxInParent is 0 When interruptible reflow is disabled, there won't be any show event for <a> tag's child text node, because CacheChildrenInSubtree() has kNoEvents for the TreeMutation. I may be wrong, but sending show events for both the accessible of <a> tag and its child text node seems reasonable. In contrary sending show event only for <a> tag's accessible is a bit strange. If anyone could help clarify this...

> I may be wrong, but sending show events for both the accessible of <a> tag > and its child text node seems reasonable. In contrary sending show event > only for <a> tag's accessible is a bit strange. If anyone could help clarify > this... that's fine, there only needs to be show events for the roots of new subtrees. So both are "correct" but obviously the interruptable reflow way takes a lot more work. I'm not really sure what to do about that, can we easily check if a part of the frame tree has any dirty frames and then block building the a11y tree on reflow there being finished? is that a good idea, or can it cause problems with some parts never being finished?

(In reply to Trevor Saunders (:tbsaunde) from comment #23) > that's fine, there only needs to be show events for the roots of new > subtrees. So both are "correct" but obviously the interruptable reflow way > takes a lot more work. Can we skip sending show events for 1) the roots of new but incomplete subtrees, or 2) the roots of new subtrees which have height 0? > I'm not really sure what to do about that, can we easily check if a part of > the frame tree has any dirty frames and then block building the a11y tree on > reflow there being finished? is that a good idea, or can it cause problems > with some parts never being finished? NS_SUBTREE_DIRTY() can be used for the checking. It seems that CreateAccessible() already blocks building the a11y tree (returns nullptr) when the text frame is dirty, but I am not sure is it true for the other kinds of frames. Also I don't really know how our layout system works, but I guess there can be a case that the document always has dirty frames, for instance modify a text every 1ms.

(In reply to Ting-Yu Chou [:ting] from comment #24) > (In reply to Trevor Saunders (:tbsaunde) from comment #23) > > that's fine, there only needs to be show events for the roots of new > > subtrees. So both are "correct" but obviously the interruptable reflow way > > takes a lot more work. > > Can we skip sending show events for 1) the roots of new but incomplete > subtrees, or 2) the roots of new subtrees which have height 0? 1 I think we could reasonably delay until the whole tree is built, but I think we need to do it eventually / within some "reasonable" time after the frames are created. 2 I don't see any reason to I don't see any reason that case can be special so no. > > I'm not really sure what to do about that, can we easily check if a part of > > the frame tree has any dirty frames and then block building the a11y tree on > > reflow there being finished? is that a good idea, or can it cause problems > > with some parts never being finished? > > NS_SUBTREE_DIRTY() can be used for the checking. It seems that > CreateAccessible() already blocks building the a11y tree (returns nullptr) > when the text frame is dirty, but I am not sure is it true for the other checking if subtrees are dirty and if so keeping them on a list of pending trees to build seems possible. > kinds of frames. Also I don't really know how our layout system works, but I > guess there can be a case that the document always has dirty frames, for > instance modify a text every 1ms. I'm not sure if that would be enough unless you are suggesting keep creating more work than can be done in one portion of the reflow, but I'm not sure what reasonable semantics that is supposed to have. Boris have any thoughts as someone more familiar with this area?

(In reply to Trevor Saunders (:tbsaunde) from comment #25) > (In reply to Ting-Yu Chou [:ting] from comment #24) > > NS_SUBTREE_DIRTY() can be used for the checking. It seems that > > CreateAccessible() already blocks building the a11y tree (returns nullptr) > > when the text frame is dirty, but I am not sure is it true for the other > > checking if subtrees are dirty and if so keeping them on a list of pending > trees to build seems possible. apparently a11y notified about subtrees becoming undirty at least in case of text frames (since there's a show event for a text node), I guess we can just ignore dirty subtrees and delay eventing until interrupted reflow is finished, if it's possible.

In general, you _can_ always have a dirty subtree. For example, you could keep doing interruptible reflows and keep getting interrupted and not really make progress. Apart from that situation, you should sometimes have entirely clean frame trees at the point when a reflow completes. At other times you may have a frame tree parts of which are clean and parts are dirty; the root will of course be NS_SUBTREE_DIRTY() in that case. What I don't know is how the timing of these dirty-bit checks that a11y wants to do correlates with the timing of reflow completion (and hence a chance to have nothing dirty).

Alexander, can you help find an owner for this bug? Is it something that will be disabled for 52 on release automatically, or do we need to land something to disable it for 52 in beta, now?

My understanding is that e10s+a11y is disabled in 52 and 53 after bugs 1325690 and 1333250.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #28) > Alexander, can you help find an owner for this bug? > Is it something that will be disabled for 52 on release automatically, or do > we need to land something to disable it for 52 in beta, now? I'm not yet sure about scope of the bug. Is non e10s build affected that bad too? If it is then nothing will happen automatically. If e10s build is affected only, then it probably will, since e10s a11y is not a default mode yet (please correct me if I'm wrong). If e10s disabled build is affected then we likely deal with event coalescence events performance problem. If not, then it could be ipc bandwith issue. It will be very helpful to have profiles for both e10s enabled and e10s disabled builds. Also I'm not clear if we need to do anything about interruptible reflow on a11y side. Say, if a11y got a notification about node A inserted, the node A contains node B which frame is dirty. If a11y ignores B and its subtree, then will a11y be notified somewhere later that B becomes undirty? Here's a list of a11y callers [1], which triggers accessible tree update for a new content. Boris, can you help please to answer the question above? [1] https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsAccessibilityService%3A%3AContentRangeInserted%28nsIPresShell+%2A%2C+nsIContent+%2A%2C+nsIContent+%2A%2C+nsIContent+%2A%29%22

(In reply to alexander :surkov from comment #30) > If e10s disabled build is affected then we likely deal with event > coalescence events performance problem. not event coalescence problem of course, but event delivering is a bottleneck then, and then we need to reduce amount of events.

I'm updating the flags to reflect that this is a non-default (user forced) configuration except on current nightly (54). Please let me know if you disagree.

Alice, would you mind trying this build? https://treeherder.mozilla.org/#/jobs?repo=try&revision=a23ea3b63d35fac3e96b614004e224ac8401b264

(In reply to Aaron Klotz [:aklotz] from comment #33) > Alice, would you mind trying this build? > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=a23ea3b63d35fac3e96b614004e224ac8401b264 The try build crashed at step3 of comment#0.... bp-b22cd77c-b5e9-48da-a861-78a6e2170222 bp-720159ad-0605-4e0b-84f3-98df02170222 (FYI, I can reproduce the hang on latest Nightly54.0a1(2017-02-21).)

Boris, could you answer the latest part of comment #30 please?

> then will a11y be notified somewhere later that B becomes undirty? I assume this is the thing you're really asking? There is no special interruptible-reflow-related notification here, I don't think. That said, how is this different from the reflow not being interrupted and then a style or content change happening that queues up a new reflow for B?

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #36) > > then will a11y be notified somewhere later that B becomes undirty? > > I assume this is the thing you're really asking? > > There is no special interruptible-reflow-related notification here, I don't > think. Ok, so we are expected in 'bad state' nodes and attempt to process them later? How does layout keeps the tracking of them? > That said, how is this different from the reflow not being > interrupted and then a style or content change happening that queues up a > new reflow for B? Accessible tree relies on a frame tree, in particular, on nsIFrame::AccessibleType(). If the reflow was interrupted, then I assume we may nor create an accessible or create a wrong type accessible. If the reflow is not interrupted, then we end up with creating a 'normal' accessible tree since frame tree is in 'good' state, all subsequent changes will just update the accessible tree.

> Ok, so we are expected in 'bad state' nodes and attempt to process them later? Probably? > How does layout keeps the tracking of them? Unwinding from an interrupted reflow ensures that it sets dirty bits up the tree as needed, and then we reschedule the reflow root whose reflow was interrupted for another reflow. So once we've unwound the whole thing it looks just like it would if the DOM had just been modified, more or less. > If the reflow was interrupted, then I assume we may nor create an accessible or create a > wrong type accessible. I just looked through our AccessibleType() impls. Looks like the ones that rely on the output of reflow are: 1) nsBlockFrame, when IsTableCaption(). This seems totally buggy to me. Consider this testcase: <table> <caption style="width: 0; height: 0">Some text</caption> </table> in this case, the caption's GetRect().IsEmpty() will return true, but do we really want this to be eNoType as opposed to eHTMLCaptionType? 2) nsFrame, when IsTableCaption(). Pretty sure this is unreached, but has the same issue anyway. 3) nsHTMLScrollFrame, when IsTableCaption(). This one is harder to call, because at least it will clip its kids when it has no size... Is there something else I'm missing? Seems to me that we should consider fixing those to not rely on layout. Note that interruptible reflow will _NOT_ affect the actual types of frames or whatnot, which is what most of the AccessibleType() impls seem to depend on.

Alex what's next here?

address Boris's comment #38 to not worry about interruptable reflows when creating accessible tree, and then figure out if we can do similar thing for text nodes to avoid extra eventing (see comment 22): currently we detect whether a text node is accessible or not by checking its rendered text, which may be affected, I assume, by interrupted reflow.

for review by trevor

so clearly I had completely forgotten what was happening here when I said in the meeting I was skeptical of it being reflow related. That said I'll summarize what I think now a little. 1. there is a regression in both e10s and non e10s caused by doing more incremental reflows in this case. 2. the only e10s vs non e10s regression here is that the method calls made on each event take longer and the sum of those hurts a lot. 3. if we speed up method calls in the e10s case that should at least mostly fix the e10s vs non e10s regression. 4. maybe we should fix the reflow regression, but I don't see why that needs to block e10s. so I think we can just add some stuff to the com handler and this will no longer be a regression in e10s vs non e10s and it can stop blocking e10sa11y. looking at the stuff that shows up in the profile and we should presumably cache we have this: get_accParent() that should be pretty easy to cache, we already know all the places in the parent where that can change and just need to send updates for it. get_accState() that would be some work, and will take some auditing, but for just the 31 msaa states is probably tractable. Next() I'm not really sure what that's doing and we should probably figure that out, but assuming its looking at sibling accessibles it seems managable to cache it.

(In reply to Trevor Saunders (:tbsaunde) from comment #42) > so I think we can just add some stuff to the com handler and this will no > longer be a regression in e10s vs non e10s and it can stop blocking e10sa11y. Filed 1363595 for the e10s+a11y part, leaving this bug for the reflow stuff.

Too late for 54 as we've built 54 RC. Mark 54 won't fix.

I'm still not sure what is actually left here to do. Do I need to track this bug? Is anyone working on it?

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #45) > I'm still not sure what is actually left here to do. Do I need to track this > bug? Is anyone working on it? I consider this bug as a complex issue that has several dependencies before it can be pronounced fixed. I believe it is on track by a11y team, but no real activity/results so far.

OK, marking fix-optional for 55 and 56, sounds like the reflow work is planned but doesn't need us to keep triaging.

I filed bug 1380451 for the final part of comment #38

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #38) > Note that interruptible reflow will _NOT_ affect the actual types of frames > or whatnot, which is what most of the AccessibleType() impls seem to depend > on. Interruptible reflow doesn't change a frame tree, i.e. it doesn't create or delete any frames, correct? In case of interruptible reflow the rendered text of text nodes may be not yet computed (aka nsIFrame::GetRenderedText() returns empty string)? If so, then is there any flag to detect that the text nodes are processed (rendered text is computed) within a document or a certain subtree?

> Interruptible reflow doesn't change a frame tree Not any more so than non-interruptible reflow. It can create/destroy continuations, obviously. > aka nsIFrame::GetRenderedText() returns empty string Yes, this would be true, I think. > If so, then is there any flag to detect that the text nodes are processed (rendered text is computed) within a document or a certain subtree? Not that I know of.

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #50) > > Interruptible reflow doesn't change a frame tree > > Not any more so than non-interruptible reflow. It can create/destroy > continuations, obviously. is it a multiple frames for a node, for example, multiple text frames spanned over multiple lines for a single text node? > > If so, then is there any flag to detect that the text nodes are processed (rendered text is computed) within a document or a certain subtree? > > Not that I know of. So no any flags on a text frame or its ancestor like some dirty bits we can use to safely assume that a text node doesn't have yet any rendered text? When rendered text computation happens, is it a separate task from other frames reflow, i.e. the rendered text is computed for multiple nodes separately from the frame trees they reside in? or is rendered text computed for a text node during a reflow of its grand-parent frame? I'm looking for a way to postpone an accessible tree creation until rendered text is ready.

> for example, multiple text frames spanned over multiple lines for a single text node? If you mean what layout data structures generally look like, yes. > So no any flags on a text frame or its ancestor like some dirty bits There are definitely dirty bits: See NS_FRAME_IS_DIRTY. Whether those do what you want, I can't tell you. I don't know how the "rendered text" is computed, sorry. I'd have to go dig through the code.

So the problem here is we fail to create an text leaf accessibles for text nodes during interrupted reflow because their text is not yet rendered, and that makes us to generate show/text changed events, coalesce and fire/deliver them, which is slow since it may happen for each text node on a page. The patch delays a11y processing to give a reflow little more time to finish: I grab a last dirty accessible (if any) and wait for a next WillRefresh to check whether the frame is still dirty. If not, then we are safe to process a11y stuff, if not, then wait for a next train. I don't experience the browser hangs actually when VoiceOver is running, but with the patch the page is definitely less sluggish. I marked a possible issue about never flushing a11y tree by XXX section, not sure if it's something real we should take care of. If it is, then I could add a flag stating whether we attempted to set WeakFrame on a previous run. Boris, could you recommend anyone check an approach and approve the layout changes (a new method in nsIPressShell)? (You don't accepting reviews).

You could try dholbert. That said, the set of dirty roots is conceptually unordered, so it's not obvious to me why the proposed API is the right thing...

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #54) > You could try dholbert. thank you > That said, the set of dirty roots is conceptually unordered, so it's not > obvious to me why the proposed API is the right thing... you mean it's not first-in-first-out structure, i.e. if a frame was a last one in the mDirtyRoots, then it can be taken out next, and WillRefresh may trigger with mDirtyRoots.RemoveElement(lastDirtyFrame)?

> you mean it's not first-in-first-out structure Right now it is, but there's no reason it has to be and it might change to not be one.

Comment on attachment 8908704 [details] [diff] [review] let interrupted reflow to finish Review of attachment 8908704 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/base/NotificationController.cpp @@ +12,5 @@ > > #include "mozilla/dom/TabChild.h" > #include "mozilla/dom/Element.h" > #include "mozilla/Telemetry.h" > +//#include "nsIFrame.h" Remove this. @@ +608,5 @@ > return; > > + // Let an interrupted reflow to process all dirty frames known at > + // previous iteration. > + // XXX: is it possible that we stuck into a loop of a previously dirty 'get stuck in a loop' or 'are stuck in a loop'. @@ +609,5 @@ > > + // Let an interrupted reflow to process all dirty frames known at > + // previous iteration. > + // XXX: is it possible that we stuck into a loop of a previously dirty > + // last frame keeping dying and when the reflow queue is never empty. 'that keeps dying and the reflow queue does not empty.' @@ +619,5 @@ > + } > + else { > + // If we are called in the middle of interrupted reflow, then hold off > + // the a11y processing until all dirty frames known to this point are > + // processed. Is it correct for this to be in an 'else' block? What if mLastDirtyRoot is not dirty anymore, but GetLastDirtyRoot() returns another dirty frame?

(In reply to Eitan Isaacson [:eeejay] from comment #57) > @@ +619,5 @@ > > + } > > + else { > > + // If we are called in the middle of interrupted reflow, then hold off > > + // the a11y processing until all dirty frames known to this point are > > + // processed. > > Is it correct for this to be in an 'else' block? What if mLastDirtyRoot is > not dirty anymore, but GetLastDirtyRoot() returns another dirty frame? mLastDirtyRoot is not dirty anymore, then we start the a11y refresh. If GetLastDirtyRoot() is not null at that point then it's also fine, we'll fire all text events on a next run, when frames will be undirty (and thus will be accessible and have text). We don't want to wait when interrupted reflows is complete, because it may never happen. So we just wait for frames that were dirty at a previous run to be processed, and that's it. (In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #56) > > you mean it's not first-in-first-out structure > > Right now it is, but there's no reason it has to be and it might change to > not be one. I see the point. The approach to delay a11y processing to give interruptible refresh more time to finish the stuff improves the performance, so if I could implement the approach by not relying on mDirtyRoots structure, then it's cool, I just not sure I have ideas how.

Eitan, does comment 58 makes any sense to you?

Comment on attachment 8908704 [details] [diff] [review] let interrupted reflow to finish Review of attachment 8908704 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me from an a11y perspective, we'll see what layout says.

Comment on attachment 8908704 [details] [diff] [review] let interrupted reflow to finish Review of attachment 8908704 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me from an a11y perspective, we'll see what layout says.

Can you give MarcoZ a try build? Also do we have measurements for this improvement? (I'd love to see)

(In reply to alexander :surkov from comment #53) > The patch delays a11y processing to give a reflow little more time to > finish: I grab a last dirty accessible (if any) and wait for a next > WillRefresh to check whether the frame is still dirty. If not, then we are > safe to process a11y stuff, if not, then wait for a next train. What happens if we have e.g. a CSS Animation (or JS-driven animation) that targets a layout-relevant property on some frame (say, "width" or "margin-left"), so that we've *always* got some reflow work to do in every WillRefresh cycle (with the same dirty root)? Would that mean we never get to whatever a11y work your patch is postponing here?

Ah, I found the answer to my question -- NotificationController is a post-layout RefreshObserver, via "AddRefreshObserver(..., FlushType::Display)". So really, your patch waits until the *end of reflow* in the next WillRefresh cycle, at which point it checks for dirty frames. OK, that makes a bit more sense. This still seems like it's separated a bit from the information we really want, though, which is "was the most recent reflow interrupted". Perhaps we could expose that more directly (say, via a bool + getter on nsIPresShell)? Or am I misunderstanding?

In fact, we have a bool "mSuppressInterruptibleReflows" that might already track "was the most recent reflow interrupted" -- it's only set to true in one spot, which is where we determine that. (Its naming doesn't so much reflect this meaning, but rather reflects how it's currently used, I guess.) So one possible solution here might be: - Add a getter for that boolean (and maybe rename it). - Skip this NotificationController stuff if the bool is set (i.e. if our most recent reflow was interrupted) - Make NotificationController track how many *consecutive* times it has skipped its business due to the bool being set -- and after that counter hits N (for some reasonable and maybe even pref-controlled N, e.g. 30), just proceed anyway. (If I'm reading this bug right, we want to be sure we eventually proceed, and proceeding too early would just mean we send extra events and slow down a bit, but we won't really do anything super-wrong, right?) BTW: reflow only gets interrupted by user input events like a window-resize or a mouse action. And only extremely large pages will take long enough to be interrupted. So for the majority of cases, we should only be able to get a few consecutive reflow interruptions, I'd think -- hence, my suggestion of a counter with a reasonable maximum.

addressed Eitan's nits and added a comment that a11y wants PresShell::mDirtyRoots to be a first-in-first-out structure.

Comment on attachment 8909787 [details] [diff] [review] let interrupted reflow to finish Review of attachment 8909787 [details] [diff] [review]: ----------------------------------------------------------------- missed Daniel's comments, canceling the review request for now

(In reply to Daniel Holbert [:dholbert] from comment #65) > So one possible solution here might be: > - Add a getter for that boolean (and maybe rename it). > - Skip this NotificationController stuff if the bool is set (i.e. if our > most recent reflow was interrupted) > - Make NotificationController track how many *consecutive* times it has > skipped its business due to the bool being set -- and after that counter > hits N (for some reasonable and maybe even pref-controlled N, e.g. 30), just > proceed anyway. (If I'm reading this bug right, we want to be sure we > eventually proceed, and proceeding too early would just mean we send extra > events and slow down a bit, but we won't really do anything super-wrong, > right?) Isn't the approach less deterministic than waiting for a last dirty frame to go away? It depends on N and if the user continues to do something like scrolling a large document, then we might exceed N sooner or later and start the accessible tree creation having no rendered text yet computed. Does it sound right?

Marco, the original approach try build is in comment 66 (will be ready in few hours)

(In reply to alexander :surkov from comment #69) > Isn't the approach less deterministic than waiting for a last dirty frame to > go away? It depends on N Sure -- the "N" was an optional suggested extension; we don't necessarily need that part. The part I care more about is exposing a bool whose meaning is clear, vs. exposing a raw nsIFrame pointer whose meaning is a bit harder to reason about. (Note that these two designs are pretty much identical in practice, too -- hence my preference for the simpler one. If it were possible for us to have lots of different roots, then the current design would make more sense perhaps -- but in practice, I think the "dirty roots" array will (nearly) always contain at most one thing, the *actual root* of the frame tree. Particularly in an initial interruptible reflow. The other possible roots are for textareas & scrollbars (so we can do targeted reflows when the user manipulates those), but those are not going to be the containers that experience very long reflows & get interrupted.) > and if the user continues to do something like > scrolling a large document, then we might exceed N sooner or later and start > the accessible tree creation having no rendered text yet computed. Yeah, let's forget about the "N" part. (I don't know how/when rendered text is computed, BTW. Maybe we really want to chain off of that more directly, if that's possible?)

(In reply to Daniel Holbert [:dholbert] from comment #71) > (In reply to alexander :surkov from comment #69) > > Isn't the approach less deterministic than waiting for a last dirty frame to > > go away? It depends on N > > Sure -- the "N" was an optional suggested extension; we don't necessarily > need that part. > > The part I care more about is exposing a bool whose meaning is clear, vs. > exposing a raw nsIFrame pointer whose meaning is a bit harder to reason > about. I see the point. Originally I was thinking to wrap it by a helper class something like class InterruptedReflowState { public: InterruptedReflowState(nsIPresShell* ps) : mDirtyRoot(ps->GetLastDirstyRoot()) {} bool IsProcessedYet() { return !!mDirtyRoot; } private: WeakFrame mDirtyRoot; }; to add an abstraction layer for a11y module, but wasn't sure it's a good idea. > (Note that these two designs are pretty much identical in practice, too -- > hence my preference for the simpler one. If it were possible for us to have > lots of different roots, then the current design would make more sense > perhaps -- but in practice, I think the "dirty roots" array will (nearly) > always contain at most one thing, the *actual root* of the frame tree. > Particularly in an initial interruptible reflow. The other possible roots > are for textareas & scrollbars (so we can do targeted reflows when the user > manipulates those), but those are not going to be the containers that > experience very long reflows & get interrupted.) bz said in comment #27 that we may never quite an interruptible reflow, so that's why I did all buzz around a last dirty frame. I'm happy to go with mSuppressInterruptibleReflows check, but not sure if we are at risk of not building an accessible tree for a document, while textareas or something keeps that flag on. Is interruptible reflow may happen on user input only? If so, then I think it should safe to simply postpone a11y tree building until interruptible reflow is finished - because the user cannot interact with a web page and depend on a11y tree same time. > > and if the user continues to do something like > > scrolling a large document, then we might exceed N sooner or later and start > > the accessible tree creation having no rendered text yet computed. > > Yeah, let's forget about the "N" part. > > (I don't know how/when rendered text is computed, BTW. Maybe we really want > to chain off of that more directly, if that's possible?) so am I, I don't know much about rendered text - only that a11y needs it. What do you mean by chaining off that more directly?

Alex, I can still reproduce the hang with the try build. I do recover from it after a few minutes, but all I need to do is load the attachment mentioned in comment #0 in a new tab while NVDA is running. NVDA's virtual buffer stays responsive, it loads the page up to the separator, but Firefox itself is veeeeeeery slow.

(In reply to Marco Zehe (:MarcoZ) from comment #73) > Alex, I can still reproduce the hang with the try build. I do recover from > it after a few minutes, but all I need to do is load the attachment > mentioned in comment #0 in a new tab while NVDA is running. NVDA's virtual > buffer stays responsive, it loads the page up to the separator, but Firefox > itself is veeeeeeery slow. Can you see any difference between builds or both of them are sluggish? I can see a difference between the builds when running VoiceOver; both builds are ok under VoiceOver, the try one feels snappier though. I guess NVDA could make more interactions with us, and we might run into other bottleneck.

It does feel a bit snappier overall in apps like Gmail, so there is definitely an improvement I think.

Daniel, ni for comment 72

(In reply to alexander :surkov from comment #72) > I'm happy to go with mSuppressInterruptibleReflows check, but not sure if we > are at risk of not building an accessible tree for a document, while > textareas or something keeps that flag on. > > Is interruptible reflow may happen on user input only? Yes, that's currently how it works. > If so, then I think > it should safe to simply postpone a11y tree building until interruptible > reflow is finished - because the user cannot interact with a web page and > depend on a11y tree same time. That makes sense to me. > so am I, I don't know much about rendered text - only that a11y needs it. > What do you mean by chaining off that more directly? I meant: assuming it's initially unavailable and then "becomes available" at some point, maybe we can/should fire a notification of some sort (or set some state that we check in a post-reflow hook, etc.) when that happens, and have that be when this bug's a11y code runs? (I'm hand-waving a bit, because as noted above, I don't know anything about the "rendered text" API that a11y is depending on here.)

Comment on attachment 8911896 [details] [diff] [review] patch Review of attachment 8911896 [details] [diff] [review]: ----------------------------------------------------------------- r-, but I think this'll be r+ with the changes requested below. ::: layout/base/nsIPresShell.h @@ +1581,5 @@ > mFontSizeInflationEnabledIsDirty = true; > } > > + /** > + * Return true if the reflow was interrupted. s/the reflow/the most recent interruptible reflow/ @@ +1584,5 @@ > + /** > + * Return true if the reflow was interrupted. > + */ > + bool IsReflowInterrupted() const { > + return mSuppressInterruptibleReflows; This doesn't really make sense right now -- the bool really needs a rename for this to be sensible. Could you add a "part 1" patch to rename the bool to something that'll make this coopting more intuitive? I'm imagining it should be renamed/documented like so: // Whether the most recent interruptible reflow was actually interrupted: bool mWasLastReflowInterrupted : 1;

(bz, could you confirm that you're OK with "mSuppressInterruptibleReflows" being renamed to "mWasLastReflowInterrupted" as suggested at the end of comment 79? And with the strategy of coopting it to solve this bug, on the theory that eventually the user will stop interacting with their browser and we'll get a chance to finish the reflow and do the a11y work here. [Or even better, the reflow will complete on its own.])

That seems ok to me, assuming current things that check it (reall just DoFlushPendingNotifications) are changed to a SuppressInterruptibleReflows() accessor that documents why we want to suppress if the last reflow was interrupted.

I'm not sure I captured bz requrest properly, so pls let me know what changes should go there

Comment on attachment 8912296 [details] [diff] [review] patch2 Review of attachment 8912296 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I think this correctly captures bz's request, yeah. Only request: could you split the NotificationController.cpp changes into their own "part 2" patch? It's always best to split up non-functional refactorings/renamings (most of this patch) from changes that are meant to impact behavior. (That's what I was meaning to request, RE the refactoring being in a "part 1" patch in comment 79.) The IsReflowInterrupted() accessor should probably appear in part 2 as well, since that's where it's first used -- though it doesn't matter too much. And please include commit messages in each patch, too. (right now looks like it's just a "hg diff")

np, I'll do it, just the changes are rather minimal. I guess Mozilla swings from one single large patch approach to multiple inter-connected patches, which is ok, but might be overkill in some cases.

Comment on attachment 8912374 [details] [diff] [review] part1 marked obsolete by mistake

Comment on attachment 8912374 [details] [diff] [review] part1 Review of attachment 8912374 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! r=me

Comment on attachment 8912375 [details] [diff] [review] part2 Review of attachment 8912375 [details] [diff] [review]: ----------------------------------------------------------------- Commit message typos: > Bug 1321960, part2 - let an interrupted reflow to finish before processing a11y, r=eeejay, f=dholber (a) s/to finish/finish/ ("let... to finish" doesn't make grammatical sense) (b) s/dholber/dholbert/ (missing final "t")

(In reply to Daniel Holbert [:dholbert] from comment #90) > Commit message typos: > > Bug 1321960, part2 - let an interrupted reflow to finish before processing a11y, r=eeejay, f=dholber > > (a) s/to finish/finish/ ("let... to finish" doesn't make grammatical sense) > (b) s/dholber/dholbert/ (missing final "t") thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/c65fce03b9e0a1a7591668f727b846675cffa44a Bug 1321960 - rename nsIPresShell::mSuppressInterruptibleReflows to mWasLastReflowInterrupted to reflect better its purpose, r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/c845865489652eadaa3f9a12736c1db56c0e6f12 Bug 1321960, part2 - let an interrupted reflow finish before processing a11y, r=eeejay, f=dholbert

https://hg.mozilla.org/mozilla-central/rev/c65fce03b9e0 https://hg.mozilla.org/mozilla-central/rev/c84586548965

Is 57 affected too and should we uplift a patch?

Comment on attachment 8912375 [details] [diff] [review] part2 Approval Request Comment [Feature/Bug causing the regression]:fairly old (regressed by interruptible reflow implementation) [User impact if declined]:performance, mostly on page loading [Is this code covered by automated tests?]:fair coverage [Has the fix been verified in Nightly?]:yes [Needs manual test from QE? If yes, steps to reproduce]: not neccessary, :jamie confirmed a perf improvement [List of other uplifts needed for the feature/fix]:yes, previous patch (patch1) [Is the change risky?]:moderate risk [Why is the change risky/not risky?]:small changes in complicated code [String changes made/needed]:no

I am not comfortable taking this kind of risks in that release. Do we have number which shows how often this happens?

(In reply to Sylvestre Ledru [:sylvestre] from comment #97) > I am not comfortable taking this kind of risks in that release. > > Do we have number which shows how often this happens? No numbers, but it depends on the user interactions, if the user makes any actions like scrolling, then he'll likely to see a problem. If not, then not.

(In reply to alexander :surkov from comment #98) > (In reply to Sylvestre Ledru [:sylvestre] from comment #97) > > I am not comfortable taking this kind of risks in that release. > > > > Do we have number which shows how often this happens? > > No numbers, but it depends on the user interactions, if the user makes any > actions like scrolling, then he'll likely to see a problem. If not, then not. the worst outcome of the patch could be that we fail to create an accessible tree which will make the browser inaccessible. So far we were only reported that the patch has positive impact on perceived performance. This might make the risk acceptable.

Jim, what do you think about that? Merci

(In reply to Sylvestre Ledru [:sylvestre] from comment #100) > Jim, what do you think about that? Merci The first patch makes code improvements while keeping the same functionality so low risk, and the second alters behavior in code that is specific to accessibility. Since this is in Nightly and is reported to work with NVDA and JAWS I'm comfortable uplifting this in 57.

Comment on attachment 8912375 [details] [diff] [review] part2 Ok, taking it as we have time for a potential backout in case of regressions

Comment on attachment 8912374 [details] [diff] [review] part1 [Triage Comment]

https://hg.mozilla.org/releases/mozilla-beta/rev/b489c9d98ec6 https://hg.mozilla.org/releases/mozilla-beta/rev/dee7ff5b7cbf

(In reply to alexander :surkov from comment #96) > [Is this code covered by automated tests?]:fair coverage > [Has the fix been verified in Nightly?]:yes > [Needs manual test from QE? If yes, steps to reproduce]: not neccessary, > :jamie confirmed a perf improvement Setting qe-verify- based on Alexander's assessment on manual testing needs and the fact that this fix has automated coverage.