Closed Bug 1349211 Opened 8 years ago Closed 7 years ago

new tab animation janky with contextual identity color decoration

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox57 --- verified

People

(Reporter: bkelly, Unassigned)

References

(Blocks 2 open bugs)

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