Closed Bug 1049551 Opened 10 years ago Closed 10 years ago

add a tab delay spinner for when we can't redraw tabs fast enough (white/black/garbage frame seen when switching between tabs with e10s)

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
e10s m3+ ---

People

(Reporter: mconley, Assigned: gw280, NeedInfo)

References

Details

Attachments

(1 file, 6 obsolete files)

While working on bug 1009628, I noticed that with the patches in that bug, I'd tab switch, and periodically see a flash of white between the tab I'd been looking at, and the tab I'd just switched to.

It's really hard to see / capture, but here's a demonstration:

https://www.youtube.com/watch?v=3AmOWJsRWxA

The browser on the left is using the async tab switching. Notice the periodic white flashes?

It wasn't enough to block the work in bug 1009628, but it's sure annoying - and I can imagine it wearing on a user's eyes even if they don't realize it.

Cc'ing BenWa, since we've discussed this thing before.
Blocks: 1009628
Blocks: e10s
Given bug 1058885 just bite me on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140912030202 CSet: 2db5b64f6d49 I'd say this is alive and kicking on trunk.
OS: Mac OS X → All
Hardware: x86 → All
Version: 30 Branch → Trunk
Assignee: nobody → davidparks21
Summary: Investigate blank frame sometimes seen when switching between tabs with e10s → Investigate blank/white/black frame or garbage seen when switching between tabs with e10s
Summary: Investigate blank/white/black frame or garbage seen when switching between tabs with e10s → Investigate white/black/garbage frame seen when switching between tabs with e10s
Assignee: davidparks21 → davidp99
Assignee: davidp99 → gwright
Blocks: 1066381
The plan here is to implement a tab spinner overlay that displays until we receive confirmation the tab has painted.
Blocks: 1071560
(In reply to Jim Mathies [:jimm] from comment #8)
> The plan here is to implement a tab spinner overlay that displays until we
> receive confirmation the tab has painted.

Is that a mitigation plan for nightly or something we're considering shipping?  Given that these pauses regularly take multiple *seconds*, that seems awful if we're considering shipping that.
Flags: needinfo?(jmathies)
Replacing a black screen with a spinner will not solve the underlying issue: the entire browser locks up (alt keys don't work to switch tabs), and every tab is undisplayable, until content is finally shown in the tab being loaded.

Every time I open a youtube tab, the entire browser is unresponsive for nine seconds.
(In reply to Sean Stangl [:sstangl] from comment #11)
> Replacing a black screen with a spinner will not solve the underlying issue:
> the entire browser locks up (alt keys don't work to switch tabs), and every
> tab is undisplayable, until content is finally shown in the tab being loaded.

Hmm, you mean the browser UI?  That's weird, I only see the content area not painting (and it usually stays white, fwiw.)

> Every time I open a youtube tab, the entire browser is unresponsive for nine
> seconds.

That is bug 1062713.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #12)
> (In reply to Sean Stangl [:sstangl] from comment #11)
> > Replacing a black screen with a spinner will not solve the underlying issue:
> > the entire browser locks up (alt keys don't work to switch tabs), and every
> > tab is undisplayable, until content is finally shown in the tab being loaded.
> 
> Hmm, you mean the browser UI?  That's weird, I only see the content area not
> painting (and it usually stays white, fwiw.)

Keyboard shortcuts don't work while the content process is in the loading state, and I mostly navigate by keyboard. The browser UI is responsive if I use the mouse.
(In reply to Sean Stangl [:sstangl] from comment #13)
> Keyboard shortcuts don't work while the content process is in the loading
> state, and I mostly navigate by keyboard. The browser UI is responsive if I
> use the mouse.

We do pass keystrokes through the content process before processing them in the parent. It probably makes sense to avoid doing that for certain important keys.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> (In reply to Jim Mathies [:jimm] from comment #8)
> > The plan here is to implement a tab spinner overlay that displays until we
> > receive confirmation the tab has painted.
> 
> Is that a mitigation plan for nightly or something we're considering
> shipping?  Given that these pauses regularly take multiple *seconds*, that
> seems awful if we're considering shipping that.

shipping, although obviously we want to improve redraw perf so this overlay only shows up rarely. But if the child process locks up for some reason we can't paint, so we need something to display. (See bug 1059494 for an example.)

Longer term multiple content processes will help, but even then we won't have one process per tab.
Flags: needinfo?(jmathies)
Blocks: 1059494
Blocks: 1075177
So after many, many, many hours wrangling with layers and having to take regular breaks to eat layer cake for recovery, I had a discussion with Matt Woodrow and he suggested that a) I was mad, and b) to stop being mad. So the patch here is a first iteration at me trying to fulfill b). 

Basically, we're now drawing the spinner in XUL. It works, and it hasn't been themed yet (nor has the gif been added to any of the packages for !osx for now). The rest of it should be right, I think.
Attachment #8500520 - Flags: review?(mconley)
Comment on attachment 8500520 [details] [diff] [review]
0001-First-attempt-at-a-spinner-for-excessively-long-page.patch

Review of attachment 8500520 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.css
@@ +76,5 @@
>  }
> +
> +.tab-delay-spinner {
> +    position: absolute;
> +    list-style-image: url(chrome://global/skin/spinner.gif);

We're likely going to want to use a static image and a CSS transform to spin the spinner:

http://hg.mozilla.org/mozilla-central/file/9ee9e193fc48/browser/themes/shared/tabs.inc.css#l99

Or, failing that, an APNG.

::: browser/base/content/tabbrowser.xml
@@ -117,4 @@
>          false
>        </field>
>        <field name="arrowKeysShouldWrap" readonly="true">
> -#ifdef XP_MACOSX

Why are you removing these ifdefs from tabbrowser.xml? How are they related to this problem? Or is this interference from another patch you've been working on?

@@ +3222,3 @@
>              let timeoutPromise = new Promise((aResolve, aReject) => {
>                timeoutId = setTimeout(() => {
> +                var box = document.createElement("box");

Instead of this, I wonder if it'd make more sense to construct a "prototype" of the spinner hidden somewhere in the browser.xul document, and clone it when we need it for a particular browser.

We also typically use "let" instead of "var".

::: toolkit/themes/osx/global/jar.mn
@@ +208,4 @@
>    skin/classic/global/tree/columnpicker.gif                          (tree/columnpicker.gif)
>    skin/classic/global/tree/folder.png                                (tree/folder.png)
>    skin/classic/global/tree/folder@2x.png                             (tree/folder@2x.png)
> +  skin/classic/global/spinner.gif                                    (../../shared/spinner.gif)

The asset is going to need to be made available for Windows and Linux as well (windows/global/jar.mn, linux/global/jar.mn)
Attachment #8500520 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> We're likely going to want to use a static image and a CSS transform to spin
> the spinner:
> 
> http://hg.mozilla.org/mozilla-central/file/9ee9e193fc48/browser/themes/
> shared/tabs.inc.css#l99
> 

I forgot to mention that this code I linked to is for the tab busy / progress spinners which use this CSS transform technique.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> We're likely going to want to use a static image and a CSS transform to spin
> the spinner:

I thought about this, but most spinners are actually animations that don't actually rotate in the geometric sense. I figured an animated image of some sort was the best choice here.

> ::: browser/base/content/tabbrowser.xml
> @@ -117,4 @@
> >          false
> >        </field>
> >        <field name="arrowKeysShouldWrap" readonly="true">
> > -#ifdef XP_MACOSX
> 
> Why are you removing these ifdefs from tabbrowser.xml? How are they related
> to this problem? Or is this interference from another patch you've been
> working on?

Accidental hunks that bled into this patch. Ignore them. Sorry.

> @@ +3222,3 @@
> >              let timeoutPromise = new Promise((aResolve, aReject) => {
> >                timeoutId = setTimeout(() => {
> > +                var box = document.createElement("box");
> 
> Instead of this, I wonder if it'd make more sense to construct a "prototype"
> of the spinner hidden somewhere in the browser.xul document, and clone it
> when we need it for a particular browser.
> 
> We also typically use "let" instead of "var".

OK.

> ::: toolkit/themes/osx/global/jar.mn
> @@ +208,4 @@
> >    skin/classic/global/tree/columnpicker.gif                          (tree/columnpicker.gif)
> >    skin/classic/global/tree/folder.png                                (tree/folder.png)
> >    skin/classic/global/tree/folder@2x.png                             (tree/folder@2x.png)
> > +  skin/classic/global/spinner.gif                                    (../../shared/spinner.gif)
> 
> The asset is going to need to be made available for Windows and Linux as
> well (windows/global/jar.mn, linux/global/jar.mn)

Yep, mentioned that in my review request comment :) I wanted to be sure that this was the correct route to go down before going ahead and cataloguing the assets for all platforms.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> Instead of this, I wonder if it'd make more sense to construct a "prototype"
> of the spinner hidden somewhere in the browser.xul document, and clone it
> when we need it for a particular browser.

Missed this comment. Are you suggesting that we keep a persistent spinner available and invisible and just make it visible whenever we need to show it? I thought about that too, but is the memory cost going to be bothersome? I don't think it'd be an issue on desktop but on B2G where every kb counts we may have some trouble?
OK, fixed a couple of the issues with the previous patch. Basically just cleaned up the patch and switched to using let. I think leaving it as an animated image is correct for now based on my previous comment. As for keeping the animation persistent, I think I can look into that if we think it makes sense? I'm not convinced right now that it does.
Attachment #8501298 - Flags: feedback?(mconley)
Attachment #8500520 - Attachment is obsolete: true
Comment on attachment 8501298 [details] [diff] [review]
0001-First-attempt-at-a-spinner-for-excessively-long-page.patch

Review of attachment 8501298 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.css
@@ +75,5 @@
>    display: none;
>  }
> +
> +.tab-delay-spinner {
> +    position: absolute;

Nit - 2 space indentation in CSS files.

::: browser/base/content/tabbrowser.xml
@@ +3236,4 @@
>  
>              let timeoutPromise = new Promise((aResolve, aReject) => {
>                timeoutId = setTimeout(() => {
> +                let box = document.createElement("box");

(In reply to George Wright (:gw280) from comment #21)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> > Instead of this, I wonder if it'd make more sense to construct a "prototype"
> > of the spinner hidden somewhere in the browser.xul document, and clone it
> > when we need it for a particular browser.
> 
> Missed this comment. Are you suggesting that we keep a persistent spinner
> available and invisible and just make it visible whenever we need to show
> it? I thought about that too, but is the memory cost going to be bothersome?
> I don't think it'd be an issue on desktop but on B2G where every kb counts
> we may have some trouble?

I think I'm just weirded out that we're doing DOM construction here.

Maybe what'd be better is to have a new tabbrowser.xml method _showBusyRemoteBrowser that takes the toBrowser, and injects the box, and stashes a reference to the box somewhere on the toBrowser.

Then, in the pre-existing MozAfterRemotePaint event listener, we call a method _hideBusyRemoteBrowser that checks for the reference to the box, and sets .hidden = true on it. Then, subsequent calls to _showBusyRemoteBrowser can check for that box, and unhide it instead of re-creating it. So we memoize.

@@ +3240,5 @@
> +                box.setAttribute("pack", "center");
> +                box.setAttribute("align", "center");
> +                let spinner = document.createElement("image");
> +                spinner.className = "tab-delay-spinner";
> +                toBrowser.parentNode.appendChild(box, toBrowser);

I'd add the spinner to the box, and _then_ append to the parentNode. Also, is the ordering right? For example, with your patch, if we hit this code path, does the findbar still sit at the bottom of the browser, or does it get pushed to the top?
Attachment #8501298 - Flags: feedback?(mconley) → feedback+
Attachment #8501298 - Attachment is obsolete: true
Attachment #8503231 - Flags: review?(mconley)
Can't you just hide the browser and draw the spinner as the tabbrowser's background image?
Oooh - yes, I like that idea more. That allows us to avoid creating a new element each time. Thanks for the suggestion, Dao!
Good times. Can you tell I'm a XUL noob? :P

I'll prepare a patch.
Attachment #8503231 - Attachment is obsolete: true
Attachment #8503231 - Flags: review?(mconley)
Attachment #8503288 - Flags: review?(mconley)
Comment on attachment 8503288 [details] [diff] [review]
0001-Bug-1049551-Add-a-tab-delay-spinner-that-s-visible-i.patch

>+  background-color: #ffffff;

This should not be needed.

>+        <parameter name="toBrowser"/>

aBrowser

>+            toBrowser.hidden = true;

I suspect visibility:hidden is going to be more efficient.

>             let timeoutPromise = new Promise((aResolve, aReject) => {
>               timeoutId = setTimeout(() => {
>+                gBrowser._showBusySpinnerRemoteBrowser(toBrowser);
>                 attemptTabSwitch(aResolve, aReject);
>               }, kTabSwitchTimeout);
>             });

Use 'this' instead of 'gBrowser'.
(In reply to Dão Gottwald [:dao] from comment #31)
> Comment on attachment 8503288 [details] [diff] [review]
> 0001-Bug-1049551-Add-a-tab-delay-spinner-that-s-visible-i.patch
> 
> >+  background-color: #ffffff;

On Linux, at least, I see a black background unless I have that.

> This should not be needed.
> 
> >+        <parameter name="toBrowser"/>
> 
> aBrowser
> 
> >+            toBrowser.hidden = true;
> 
> I suspect visibility:hidden is going to be more efficient.
> 
> >             let timeoutPromise = new Promise((aResolve, aReject) => {
> >               timeoutId = setTimeout(() => {
> >+                gBrowser._showBusySpinnerRemoteBrowser(toBrowser);
> >                 attemptTabSwitch(aResolve, aReject);
> >               }, kTabSwitchTimeout);
> >             });
> 
> Use 'this' instead of 'gBrowser'.
That doesn't seem to make much sense, as we already set the background color to browser.display.background_color, which is white:
http://hg.mozilla.org/mozilla-central/annotate/097821fd89ed/browser/base/content/tabbrowser.xml#l3135
http://hg.mozilla.org/mozilla-central/annotate/50b689feab5f/modules/libpref/init/all.js#l180
Maybe I'm conflating it with when the background was set by the box element. Let me double check.
(In reply to Dão Gottwald [:dao] from comment #31)
> 
> >+            toBrowser.hidden = true;
> 
> I suspect visibility:hidden is going to be more efficient.
> 

Not sure what's being suggested here... are you saying that we should manually set the visibility style property on this node instead? Or somehow do this in CSS?
Flags: needinfo?(dao)
OK yes, so it was only hte box element which makes sense. However, I found another issue - using gBrowser instead of this seems to stop it from working. I don't know enough about scoping in js/xul to determine what the issue is.
Addressed Dao's review comments, except for the gBrowser one. I'm stumped. If I dump out the contents of "this" and "gBrowser" they look the same, but if I change gBrowser to this, it seems to break. I'm not entirely sure what's going on here.
Attachment #8503288 - Attachment is obsolete: true
Attachment #8503288 - Flags: review?(mconley)
Attachment #8503318 - Flags: review?(mconley)
Attachment #8503318 - Flags: review?(dao)
I also fixed an issue with the previous iteration where we'd only ever hide/show one of the browsers. Now we always show/hide the browser which is delayed, but only ever start/stop the spinner when the number of browsers delaying is the correct number.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #35)
> (In reply to Dão Gottwald [:dao] from comment #31)
> > 
> > >+            toBrowser.hidden = true;
> > 
> > I suspect visibility:hidden is going to be more efficient.
> > 
> 
> Not sure what's being suggested here... are you saying that we should
> manually set the visibility style property on this node instead? Or somehow
> do this in CSS?

Set an attribute on the browser and use that to set visibility:hidden in CSS.

(In reply to George Wright (:gw280) from comment #36)
> OK yes, so it was only hte box element which makes sense. However, I found
> another issue - using gBrowser instead of this seems to stop it from
> working. I don't know enough about scoping in js/xul to determine what the
> issue is.

The second occurrence of gBrowser is within a function that creates a new scope. You'd need to replace that with something like this:

  let onRemotePaint = () => {
    toBrowser.removeEventListener("MozAfterRemotePaint", onRemotePaint);
    ...
  };
  toBrowser.addEventListener("MozAfterRemotePaint", onRemotePaint);
Flags: needinfo?(dao)
OK, I think this should be it.
Attachment #8503318 - Attachment is obsolete: true
Attachment #8503318 - Flags: review?(mconley)
Attachment #8503318 - Flags: review?(dao)
Attachment #8503416 - Flags: review?(mconley)
Comment on attachment 8503416 [details] [diff] [review]
0001-Bug-1049551-Add-a-tab-delay-spinner-that-s-visible-i.patch

>--- a/browser/base/content/tabbrowser.css
>+++ b/browser/base/content/tabbrowser.css

>+browser[hideForSpinner] {
>+  visibility: hidden;
>+}
>+
>+tabbrowser[waitingOnContentPaint] {

Can you use the same attribute name for both? Also, please make this lowercase. E.g.: pendingpaint

>+  background-image: url(chrome://global/skin/spinner.gif);
>+  background-repeat: no-repeat;
>+  background-attachment: fixed;

I don't think background-attachment: fixed; is needed here.

>+  background-position: center center;
>+}
>+

Remove the trailing line.

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml

>             let paintPromise = new Promise((aResolve, aReject) => {
>-              toBrowser.addEventListener("MozAfterRemotePaint", function onRemotePaint() {
>+
>+              let onRemotePaint = () => {

Remove the blank line.

>                 toBrowser.removeEventListener("MozAfterRemotePaint", onRemotePaint);
>+                this._hideBusySpinnerRemoteBrowser(toBrowser);
>                 clearTimeout(timeoutId);
>                 attemptTabSwitch(aResolve, aReject);
>-              });
>+              };
>+
>+              toBrowser.addEventListener("MozAfterRemotePaint", onRemotePaint);

Remove the blank line.

The gif is temporary and will be replaced with an APNG or CSS animation, right?

r=me with those things addressed
Attachment #8503416 - Flags: review?(mconley) → review+
Comment on attachment 8503416 [details] [diff] [review]
0001-Bug-1049551-Add-a-tab-delay-spinner-that-s-visible-i.patch

Review of attachment 8503416 [details] [diff] [review]:
-----------------------------------------------------------------

We definitely shouldn't land with the current spinner.gif... as much as I love joespider, I think it'd be silly to land that in production code.

Maybe use an image from http://preloaders.net/ until we can get something better from UX?
Madhava, can you suggest an appropriate spinner animation for this?
Flags: needinfo?(madhava)
r+ carried from Dao as per previous comment. Waiting on comment from :madhava re. the spinner (yes, joe-spider is temporary).
Attachment #8503416 - Attachment is obsolete: true
Attachment #8504879 - Flags: review+
As much as I really, really, really wanted to see :joedrew dancing across thousands of our users' browser windows, I couldn't bring myself to do it. So it's landed, but with a spinner from preloaders.net.

https://hg.mozilla.org/integration/mozilla-inbound/rev/869f6077817a
(In reply to George Wright (:gw280) from comment #45)
> As much as I really, really, really wanted to see :joedrew dancing across
> thousands of our users' browser windows, I couldn't bring myself to do it.
> So it's landed, but with a spinner from preloaders.net.

What's the license for those spinners?
(In reply to Dão Gottwald [:dao] from comment #47)
> What's the license for those spinners?

http://preloaders.net/en/terms_of_use
Flags: needinfo?(gwright)
To clarify, I did check those terms of use before committing, and I believe I was in compliance, but if we have any additional steps from a Mozilla perspective that I should have adhered to then I am unaware of them.
OK, so I think I know the root cause of this now. I think browser_windowopen_reflows.js is never getting the MozAfterPaint event because the event is never sent as the tabbrowser is visibility:hidden. Using my original patch where the image is overlayed solves this.

I personally think that the original method whereby the image is overlayed is better here. What do you think, Dao? The code from the previous patch will obviously need a bit of cleaning up, but I have a better idea of the xul/js coding style now.
Flags: needinfo?(dao)
Can you cheat and replace visibility:hidden with opacity:0 or, if that doesn't work either, some low value like opacity:0.001?
Flags: needinfo?(dao)
Curious, opacity:0 appears to work. Let's try that!
Oh, one thing slipped through in my review. Don't put the spinner in toolkit, put it in browser/themes/*/tabbrowser/ instead.
... and rename it to pendingpaint.png while you're at it.
(In reply to Dão Gottwald [:dao] from comment #57)
> ... and rename it to pendingpaint.png while you're at it.

Oops, landed before I saw this comment. Let's leave it as is for now, and address it when UX replaces the spinner with their own one.
You missed comment 56 too. Please address this ASAP.
Flags: needinfo?(gwright)
https://hg.mozilla.org/mozilla-central/rev/30b100f7edbe
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
(In reply to Dão Gottwald [:dao] from comment #60)
> You missed comment 56 too. Please address this ASAP.

George?
Depends on: 1087007
Backed out.

https://hg.mozilla.org/integration/fx-team/rev/2bf47b8cfdc0
Status: RESOLVED → REOPENED
Flags: needinfo?(gwright)
Resolution: FIXED → ---
Attachment #8504879 - Flags: review+ → review-
I figured that as we'd have to change the animation anyway we'd just fix it when the new animation is ready from UX, as per comment 59...
https://hg.mozilla.org/mozilla-central/rev/1db368e224b7
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Summary: Investigate white/black/garbage frame seen when switching between tabs with e10s → add a tab delay spinner for when we can't redraw tabs fast enough (white/black/garbage frame seen when switching between tabs with e10s)
For historical reference: see also Bug 1087934, which is where pendingpaint.png gets added to the aero part of browser/themes/windows/jar.mn (for Firefox 37).
Any way to disable this feature, for example with userChrome (I was trying but can't find a proper way to revers the changes)?

When switching/closing multiple tabs it's blinking white page very fast and it may not be good for user experience. I rather see garbaged pages that don't jump from dark colors to white too often - it gets me headaches.

Maybe some other solution?
You need to log in before you can comment on or make changes to this bug.