Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Description • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 1 • 9 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 2 • 9 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 3 • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 4 • 9 years ago

(ResizeReflowOverride was added in Bug 596969.)

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 5 • 9 years ago

Can repro on Windows.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 6 • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 7 • 9 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 8 • 9 years ago

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.)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 9 years ago

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()

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 10 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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)

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 11 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 12 • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 13 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 14 • 9 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 15 • 9 years ago

Oh, maybe...

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 17 • 9 years ago

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.)

	Maksim Lebedev
	Comment 18 • 9 years ago

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.

	Maksim Lebedev
	Comment 19 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 20 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 21 • 9 years ago

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).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 22 • 9 years ago

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

	Maksim Lebedev
	Comment 23 • 9 years ago

+ 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.

	Maksim Lebedev
	Comment 24 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 25 • 9 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 26 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 27 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 28 • 9 years ago

Fixed by bug 1178847.