Closed Bug 1158640 Opened 7 years ago Closed 7 years ago

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

Categories

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

36 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s - ---

People

(Reporter: smaug, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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.
This is on Linux 64, Nightly 40.0a1 (2015-04-26)
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
Summary: [E10s] When resizing a window, all the tabs are resized → [E10s][APZ] When resizing a window, all the tabs are resized, which is very slow
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.
(ResizeReflowOverride was added in Bug 596969.)
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.
Assignee: nobody → bugs
Actually, not sure what is the best way to fix this. We have so weird and complicated setup in TabChild and in ViewManager.
Assignee: bugs → nobody
Attached patch wip (obsolete) — Splinter Review
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.)
Attachment #8601792 - Flags: feedback?(bugmail.mozilla)
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()
Attachment #8601792 - Flags: feedback?(bugmail.mozilla) → feedback+
(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)
Attached patch v1Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf3001debda
Assignee: nobody → bugs
Attachment #8601792 - Attachment is obsolete: true
Attachment #8602165 - Flags: review?(bugmail.mozilla)
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.
Attachment #8602165 - Flags: review?(bugmail.mozilla)
Hmm, and we don't have tests detecting that :/
I don't think I have really time for debugging this more.
Assignee: bugs → nobody
I can take it next week if nobody else picks it up first.
Oh, maybe...
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.
Assignee: nobody → bugmail.mozilla
Blocks: apz-linux
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.)
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.
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Flags: needinfo?(bugmail.mozilla)
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).
As part of the same TabChild::RecvUpdateDimensions call the viewmanager's delayed resize also gets called. Backtrace to that attached.
+ 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.
Attachment #8627735 - Flags: feedback?(bugs)
Attachment #8627735 - Flags: feedback?(bugmail.mozilla)
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.
Flags: needinfo?(bugs)
Flags: needinfo?(bugmail.mozilla)
I have objections to doing this short-term hack instead of a proper maintainable solution which I am actively working on.
Flags: needinfo?(bugmail.mozilla)
Attachment #8627735 - Flags: feedback?(bugmail.mozilla)
The patch doesn't look bad, but if kats wants to fix this in some other way, better to wait for that fix.
Flags: needinfo?(bugs)
Attachment #8627735 - Flags: feedback?(bugs)
The patch on bug 1178847 appears to fix this; I'll close this bug once that hits m-c.
Fixed by bug 1178847.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.