1158640 - [E10s][APZ] When resizing a window, all the tabs are resized, which is very slow

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Seems like we have regressed the behavior since Bug 1096076.
When having a window with lots of tabs open (in my case ~200), resizing makes child process
to be blocked for ages, since seems like all the tabs handle RecvUpdateDimensions, which
calls HandlePossibleViewportChange, which calls SetCSSViewport which then calls
ResizeReflowIgnoreOverride.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This is on Linux 64, Nightly 40.0a1 (2015-04-26)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Ahaa, is this a regression from apz? I have that enabled.
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=78a291fd62b3&mark=270-272#268

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

So TabChild shouldn't use nsLayoutUtils::SetCSSViewport, but do the resize reflow via
ViewManager. That way it would get the normal 'pending resize' behavior, which means resize reflow is done when actually needed.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(ResizeReflowOverride was added in Bug 596969.)

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Can repro on Windows.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I was thinking this yesterday, and perhaps we need the nsPresShell::mViewportOverridden to be
moved to ViewManager, and always make the resizeReflow type of calls via ViewManager.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Actually, not sure what is the best way to fix this. We have so weird and complicated setup in TabChild and in ViewManager.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2296318ee2dd

Not happy that e10s and non-e10s have so different code paths, but that TabChild/APZ stuff and ViewManager code should be somehow merged, in some other bug ;)

Anyhow, this is a wip to cache the old size so we end up then resizing from old
to the most recent size when docshell/presshell is activated again.


(This bug happens to be the most disturbing when one has apz enabled and hundreds of tabs open, so wanted to have
some kind of fix.)

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8601792 [details] [diff] [review]
wip

Review of attachment 8601792 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +267,5 @@
> +void
> +TabChildBase::HandlePossibleViewportChange()
> +{
> +  ScreenIntSize oldInnerSize =
> +    mOldInnerSize.IsEmpty() ? mInnerSize : mOldInnerSize;

I'd rather you made mOldInnerSize a Maybe<ScreenIntSize> so that the set/unset state is more explicit.

@@ +2063,5 @@
> +      nsCOMPtr<nsIPresShell> shell = GetPresShell();
> +      if (shell && !shell->IsActive()) {
> +        // Cache the value for the case when the shell becomes active again.
> +        if (mOldInnerSize.IsEmpty()) {
> +          mOldInnerSize = oldScreenSize;

Shouldn't this unconditionally clobber mOldInnerSize? I don't understand why you check for IsEmpty()

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> I'd rather you made mOldInnerSize a Maybe<ScreenIntSize> so that the
> set/unset state is more explicit.
Ah, yes, that is nicer. No need to rely on width or height being 0 (which IsEmpty relies on).

> Shouldn't this unconditionally clobber mOldInnerSize? I don't understand why
> you check for IsEmpty()
No, I want to resize from the old size to the new one, and not some recent-ish size, which we aren't in to the latest one.
(scaling seemed to get wrong if one doesn't use the right sizes)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf3001debda

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8602165 [details] [diff] [review]
v1

Review of attachment 8602165 [details] [diff] [review]:
-----------------------------------------------------------------

The patch makes sense but it causes some weird behaviour in B2G. STR:
1) Open the browser
2) Load a page
3) Open a new browser window, it will show about:home
4) Use the edge swipe to go to the first window
5) Rotate
6) Use the edge swip to go to the second window

The about:home display in the second window doesn't get resized and so it is either too small or too big.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Hmm, and we don't have tests detecting that :/
I don't think I have really time for debugging this more.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I can take it next week if nobody else picks it up first.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Oh, maybe...

Comment 16

[Kartikaya Gupta (email:[e-mail])]

I didn't get to this when I said I would, but I do want to fix this before enabling APZ on nightly as it will likely affect the user experience significantly. I plan on reorganizing much of the code dealing with the CSS viewport for fennec anyway so I intend to fix this up as part of that.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

This is indeed the most annoying apz bug on linux. (Otherwise I'm rather pleased with APZ, and have been using it now for 2 months on linux.)

Comment 18

[Maksim Lebedev]

What is the current state of bug. Can I make something on it?

I can provide the idea of suppressing RecvUpdateDimensions on TabChild if mPuppetWidget is not visible.
And then mPuppetWidget become visible we can once generate notification for change sizes.

Comment 19

[Maksim Lebedev]

Continuation of idea:
When we change active tab we receive TabChild::RecvSetIsDocShellActive() and come into PuppetWidget::Show(). And at this point we can change previous size to real size (or size, which we got the latest) and call Resize(). It will be more simply then change all of docShell or viewManager code.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

I'm actively working on this bug. As this code is used heavily on B2G and is very likely to break things there, I would not recommend you spending much more time on this unless you have managed to get a Firefox OS device that you can test on. Unfortunately there are very few tests for this code so even a green try run is not sufficient to land changes in this area.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backtrace to resize event

This is the backtrace to the resize event that gets fired upon resizing. As I suspected it goes through HandlePossibleViewportChange (aka the root of all evil).

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backtrace to viewmanager resize

As part of the same TabChild::RecvUpdateDimensions call the viewmanager's delayed resize also gets called. Backtrace to that attached.

Comment 23

[Maksim Lebedev]

Attachment: tab_resizing_issue_ver1.diff

+ Added small cache of updated dimensions for not-active tab
+ Added check of updated dimensions in case tab activating

In case assuming of enabling APZ I can provide idea of very fast fix current issue without big changes in code at ViewManagers or presShells - such changes can be made after enabling APZ. So I hope my idea of fix is understandable.

Suggestions and comments and objections are very welcome.

Comment 24

[Maksim Lebedev]

Unfortunately, I have no FFOS devices for test my patch, but I assume that it correctly work on all platforms because it does not change any important code.
So if anybody can confirm or disapprove my opinion - it will be good. Feel free to make such testing.

Suggestions and comments and objections are still very welcome.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

I have objections to doing this short-term hack instead of a proper maintainable solution which I am actively working on.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

The patch doesn't look bad, but if kats wants to fix this in some other way, better to wait for that fix.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

The patch on bug 1178847 appears to fix this; I'll close this bug once that hits m-c.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Fixed by bug 1178847.