1349211 - new tab animation janky with contextual identity color decoration

Status: RESOLVED WORKSFORME

(Firefox :: Tabbed Browser, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

When using contextual identities or "containers" in nightly each tab gets a colored bar at the top.  I noticed that the new tab animation (tab sliding out to right from new tab button) is janky when opening a container tab.

Sorry, I'm not sure where to file this.  Its not really a DOM security issue like the rest of the contextual identity stuff, but I also don't see a UX component for the feature.

Comment 1

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

I have tested your issue on latest Nightly build (Buid ID: 20170403030207) and managed to reproduce it. When opening a new container tab by long click on "+" button, the browser/image flickers and the tab animation is janky.

Comment 2

[Peter Chang[:pchang]]

(In reply to Vlad Bacia-Mociran [:VladB] from comment #1)
> User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:55.0) Gecko/20100101
> Firefox/55.0
> 
> I have tested your issue on latest Nightly build (Buid ID: 20170403030207)
> and managed to reproduce it. When opening a new container tab by long click
> on "+" button, the browser/image flickers and the tab animation is janky.

I couldn't reproduce this on Windows with latest nightly.
The step I tried
a. open FF nightly
b. long click on "+" button
c. Choose personal type from a small popup
d. Saw a new tab append in the right side <--Do you mean this tab animation?

Vlad, if you can reproduce this issue, could you share a profiler with [1] for it?


[1]https://perf-html.io/

Comment 3

[Vlad Bacia-Mociran [:VladB]]

Sure(In reply to Peter Chang[:pchang] from comment #2)
> Vlad, if you can reproduce this issue, could you share a profiler with Gecko Profiler
> for it?

Sure. Here is the link https://perfht.ml/2nRzoly for the performance profile.

Comment 4

[Florian Quèze [:florian]]

From a quick look at the profile in comment 3, it seems we do several synchronous layout flushes, due to moving the focus a few times. First from tabbrowser.xml's _adjustFocusAfterTabSwitch method, and then from utilityOverlay.js' openLinkIn function.

Comment 5

[Bas Schouten (:bas.schouten)]

There doesn't appear to be anything graphics related in this profile. There's also some nsSoundPlayer::SoundReleaser::Run() taking quite a long down to shutdown its child thread (although it continues processing some events during this time). I'm not sure how to interpret this in the profile and whether there's a 'real' problem here.

Comment 6

[Peter Chang[:pchang]]

I also didn't see the bottleneck in graphic side from the profiler in comment 3.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> From a quick look at the profile in comment 3, it seems we do several
> synchronous layout flushes, due to moving the focus a few times. First from
> tabbrowser.xml's _adjustFocusAfterTabSwitch method, and then from
> utilityOverlay.js' openLinkIn function.

Both of which seem unrelated to contextual identity stuff, don't they?

Comment 8

[(no longer active)]

(In reply to Dão Gottwald [::dao] from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #4)
> > From a quick look at the profile in comment 3, it seems we do several
> > synchronous layout flushes, due to moving the focus a few times. First from
> > tabbrowser.xml's _adjustFocusAfterTabSwitch method, and then from
> > utilityOverlay.js' openLinkIn function.
> 
> Both of which seem unrelated to contextual identity stuff, don't they?

Yeah, I think this bug is just that openLinkIn needs to be improved, it's not related to graphics or contextual identity in any way that I can see.

Comment 9

[Dão Gottwald [:dao]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #8)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #4)
> > > From a quick look at the profile in comment 3, it seems we do several
> > > synchronous layout flushes, due to moving the focus a few times. First from
> > > tabbrowser.xml's _adjustFocusAfterTabSwitch method, and then from
> > > utilityOverlay.js' openLinkIn function.
> > 
> > Both of which seem unrelated to contextual identity stuff, don't they?
> 
> Yeah, I think this bug is just that openLinkIn needs to be improved, it's
> not related to graphics or contextual identity in any way that I can see.

Well, it was reported as a contextual identity issue. Seems like we're not on the same page if Ben can reproduce an issue with contextual identity enabled but not without it.

Comment 10

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [::dao] from comment #9)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #8)
> > (In reply to Dão Gottwald [::dao] from comment #7)
> > > (In reply to Florian Quèze [:florian] [:flo] from comment #4)
> > > > From a quick look at the profile in comment 3, it seems we do several
> > > > synchronous layout flushes, due to moving the focus a few times. First from
> > > > tabbrowser.xml's _adjustFocusAfterTabSwitch method, and then from
> > > > utilityOverlay.js' openLinkIn function.
> > > 
> > > Both of which seem unrelated to contextual identity stuff, don't they?
> > 
> > Yeah, I think this bug is just that openLinkIn needs to be improved, it's
> > not related to graphics or contextual identity in any way that I can see.
> 
> Well, it was reported as a contextual identity issue. Seems like we're not
> on the same page if Ben can reproduce an issue with contextual identity
> enabled but not without it.

I haven't looked at the profiles today, but if I remember  correctly, I was under the impression that opening a tab with a contextual identity calls openLinkIn whereas opening a normal tab doesn't. If so the fix here would be either to optimize openLinkIn, or make opening a contextual identity tab not call it and instead reimplement the subset of it that's relevant to its  needs.

Comment 11

[Dão Gottwald [:dao]]

We do call openLinkIn (via openUILinkIn) in the non-contextual-identity case: http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/browser/base/content/browser.js#2145

It's however possible that openLinkIn takes a different path due to different parameters or other factors.

Comment 12

[Abe - QA (:Abe_LV)]

Based on IRC chat with Florian, I am moving this to "Firefox::Tabbed Browser" component.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Hey bkelly, when you first reported this bug, were you using the Containers Test Pilot experiment at the time?

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

No, nightly containers.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

So bkelly sent me some profiles:

Here's opening a new container tab: https://perfht.ml/2swvzIo
Here's opening a new normal tab: https://perfht.ml/2swz9T8

Notice that we're painting twice in the container tab case.

Here's what's going on:

We preload the about:newtab invisibly in the background so that we can have it ready ASAP when the user wants it. We don't do this for container tabs though. This means that when a container tab is about to be opened, we do a full about:newtab load in that container, and a full paint.

I had bkelly disable preloading by setting "browser.newtab.preload" to false, and then opening up a new tab and closing it to flush the preloaded one. When he next tried to open a normal new tab, here's the profile:

https://perfht.ml/2swDHIR

And apparently, the jank was the same.

So here's the good news: Activity Stream is supposed to replace about:newtab, and is supposed to run in the content process. Assuming that is ready in time for 57, that should take care of the main process UI jank when opening up a container tab.

Hey ursula, is there a bug somewhere for turning Activity Stream on by default on Nightly somewhere, so I can add that as a blocker to this bug?

Comment 16

[Ursula Sarracini (:ursula)]

There is not currently a Bugzilla bug tracking the turning on of Activity Stream by default in Nightly for 57. All of our issue tracking is done on github (https://github.com/mozilla/activity-stream/issues). But I'll file that bug right now on Bugzilla and update the blocker on this bug.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

mconley: is there any intent in the future to try and preload container tabs?  or is that impractical?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dan Mosedale (:dmose) from comment #17)
> mconley: is there any intent in the future to try and preload container
> tabs?  or is that impractical?

As far as I know, no - unless it's possible to have a "default container", in which case, we probably always want to preload the default.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

I suspect this will be fixed in tomorrow's Nightly now that Activity Stream is enabled by default.

Activity Stream moves about:newtab loading into the content process, which should reduce the jank in the parent process when opening a new about:newtab for the first time (with or without a container).

For QA:

To verify this, you'll need to install the Test Pilot add-on, and the Containers experiment. In the "bad" case, the tab open animation should stick / jank when opening a new container tab. In the "good" case, there should be no jank - the tab should open more smoothly.

Comment 20

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 21

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170921100141 

I can reproduce this issue on Firefox Nightly Build ID: 20170403030207 on Windows 8.1 x64.

This issue has been verified on latest Firefox Nightly Build ID: 20170921100141 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04 and I cannot reproduce it. Now, when opening a new container tab the animation is smooth. Also, tried this with the Containers experiment from Test Pilot and the result was the same (when opening a new container tab the animation was smooth).

Comment 22

[Adrian Florinescu [:aflorinescu]]

Thanks Vlad, based on comment 21, marking as WFM.