new tab animation janky with contextual identity color decoration

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
2 years ago
2 years ago

People

(Reporter: bkelly, Unassigned)

Tracking

(Blocks 2 bugs)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

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.
See Also: → 921673
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.
Component: Untriaged → Graphics
Product: Firefox → Core
(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/
Flags: needinfo?(vlad.bacia)
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.
Flags: needinfo?(vlad.bacia) → needinfo?(howareyou322)
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.
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.
Component: Graphics → Layout
Flags: needinfo?(ehsan)
I also didn't see the bottleneck in graphic side from the profiler in comment 3.
Flags: needinfo?(howareyou322)
(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?
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-performance]
(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.
Flags: needinfo?(ehsan)
(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.
(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.
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.
Depends on: 1356858
Based on IRC chat with Florian, I am moving this to "Firefox::Tabbed Browser" component.
Component: Layout → Tabbed Browser
Product: Core → Firefox
See Also: → 1358813
Priority: P2 → P3
Whiteboard: [photon-performance] → [reserve-photon-performance]
Flags: needinfo?(mconley)
Hey bkelly, when you first reported this bug, were you using the Containers Test Pilot experiment at the time?
Flags: needinfo?(mconley) → needinfo?(bkelly)
No, nightly containers.
Flags: needinfo?(bkelly)
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?
Flags: needinfo?(usarracini)
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.
Flags: needinfo?(usarracini)
Depends on: 1373321
mconley: is there any intent in the future to try and preload container tabs?  or is that impractical?
(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.
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
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.
Whiteboard: [reserve-photon-performance] → [reserve-photon-performance][qa-commented]
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).
Thanks Vlad, based on comment 21, marking as WFM.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: qe-verify+
Resolution: --- → WORKSFORME
Priority: P3 → --
QA Contact: adrian.florinescu
Whiteboard: [reserve-photon-performance][qa-commented]
You need to log in before you can comment on or make changes to this bug.