1159827 - [e10s] We always show the spinner when closing a tab

Status: RESOLVED WORKSFORME

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This tends to bug me since the spinner usually shows for just a few milliseconds, and it looks really glitchy. I think that before bug 1132072, we probably would have just drawn garbage or a flat color during this period. Fixing this could be a bit tricky since we would need to keep the closed tab around long enough for the tab we're switching to to be ready.

Comment 1

[George Wright (:gw280) (needinfo me!)]

m7 -> m8 as per discussion with billm

Comment 2

[Gabor Krizsanits (INACTIVE)]

I'm taking this from George.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Hey Bill, I don't think I can reproduce this one (not even a debug build), maybe my machine is just too fast. Is this something you still experience?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I still see weird flashing when I close tabs during a compile. I'm guessing it has something to do with this bug, but I can't say for sure.

Here's an STR for something I see that definitely looks bad.

1. Make a web page that has an infinite loop in JS.
2. Open the web page.
3. Close the tab showing the slow web page.

Here's what I see:

1. First, for some bizarre reason, the new tab page flashes on screen very quickly.
2. Then I get a spinner.

I think there are two things we can do here:

1. Figure out why the new tab page appears and fix that. It's really weird.
2. Wait for 300ms or whatever the usual time is before showing the spinner.

I don't know how hard #1 is. I know #2 is hard because the code is tricky. I would recommend looking into #1 first. If even just that were fixed, I would be a lot happier with our whole tab closing experience.

Comment 5

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

I think I can answer (1).

As a performance optimization, after the first New Tab page is opened, we preload the next New Tab page in the background. This preloading works by appending a <xul:browser> to the <xul:tabpanels> that has no associated <xul:tab>. When the next New Tab is opened, we grab that preloaded <xul:browser> and associate it with the freshly opened <xul:tab>, and the preloading starts again (after, I believe, a small delay).

If the user is closing the last tab, and they've got a preloaded New Tab, then it is certainly plausible that in the time that we wait for the tab to the left of the closed tab to show its frame, that we show the preloaded browser instead (since it would have been the node right after the one that had just been removed).

A way of testing this theory is to turn New Tab preloading off, which can be done by setting browser.newtab.preload to false. If that value is false, and (after a restart) you still see the New Tab page appearing briefly when closing tabs, then my theory is bunk.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, toggling that pref makes the new tab page not show up. I'd be happy if we morph this bug so that it's about fixing the new tab issue. The #1 part we can do after M8 I think.

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #4)
> I still see weird flashing when I close tabs during a compile. I'm guessing
> it has something to do with this bug, but I can't say for sure.

I actually tried a release build before... After changing hardware I have not noticed it. With a debug build I can see it sometimes too, but I'm not sure that alone qualifies it as an m8. But let's see how far we can get with this anyway.

> 
> Here's an STR for something I see that definitely looks bad.
> 
> 1. Make a web page that has an infinite loop in JS.
> 2. Open the web page.
> 3. Close the tab showing the slow web page.

Well... this was an interesting experience. What is the exact infinite loop you are using? I tried a tight loop, a simple for(;;) console.log("something"); and it blocks the content process as expected. Weird thing is that if I close the tab the content process remains blocked, so the other side never shows up and the cpu just keeps spinning on 100%. Pre-e10s this was not an issue, because the whole browser froze and you have to stop the script first and only then could you close the tab itself. I think this should be fixed.

If I just try to keep the cpu busy with a setIntervall(..., 0) then I cannot reproduce anything interesting.
I'll try to combine the two, to see if that helps to reproduce it (non-infinite tight loop called frequently by setIntervall).

> 2. Wait for 300ms or whatever the usual time is before showing the spinner.

Could someone point me to this spinner code?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The loop I used is just |while (true);|. When you close the tab, you should eventually see a spinner. After ten seconds the slow script UI will pop up and you can stop the script. At that point everything should go back to normal.

That's kind of beside the point though. You could instead make a loop that spins for 100ms when the page is closed. That's something a real page might do. Then you'll still see this wacky new tab page behavior and an immediate spinner, which we don't want.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #8)
> The loop I used is just |while (true);|. When you close the tab, you should
> eventually see a spinner. After ten seconds the slow script UI will pop up
> and you can stop the script. At that point everything should go back to
> normal.

Oh, I think it tricked me that it does not happen in debug build. Yupp, works fine in release build (the stop script dialog I mean).

> 
> That's kind of beside the point though. You could instead make a loop that
> spins for 100ms when the page is closed. That's something a real page might
> do. Then you'll still see this wacky new tab page behavior and an immediate
> spinner, which we don't want.

Yeah I think I've got it now, thanks!

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #6)
> Yeah, toggling that pref makes the new tab page not show up. I'd be happy if
> we morph this bug so that it's about fixing the new tab issue. The #1 part
> we can do after M8 I think.

I think you mean #2 can wait.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> > spins for 100ms when the page is closed. That's something a real page might
> > do. Then you'll still see this wacky new tab page behavior and an immediate
> > spinner, which we don't want.

I don't think we can do much about the spinner easily. If the current page is spinning switching tab behaves the same way (so not just tab closing). We show the spin until the JS stops. I think for that the reason is that we always set the state of the requested tab to loading [1]. If the page is already loaded it would be a quick operation to request the layers but if the JS is blocking we cannot process the event I guess. We only cache the layers for a short time, if it's still cached we could use it, but that would help rarely. This means if JS is blocking one page we cannot switch to another even if that one is loaded, this bothers me much.

Anyway, I'm going to look into that newtab problem next, that sounded easier to fix.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3306

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, the typical case is that we have layers for the tab being closed. We just want to keep drawing those until we get layers for the new tab, or until 300ms passes.

But I agree that the newtab problem is a higher priority.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: prevent the pre-loaded browser to be displayed. v1

This bug has been driving me crazy. What I concluded that it is not the tab switcher that misbehaves. It never selects the preloaded browser accidentally or anything like that. In the state where the artifact happens the closing tab is already removed from its container and destroyed (as well as its layers) and we have not yet received the layers from the to be shown tab (this is delayed because the content process is busy, so we don't have a chance to process the corresponding event). In this state the preloaded tab is rendered to the screen briefly (I have not found any explanation why since the to be shown layer is already set as the selected and visible and even last visible tab, and we should really show its spinner instead of the preloaded browser). To avoid this I just set the spinner attribute on the preloaded browser when created and remove it when we actually want to use it. This fixes the bug, does not come with any known side effects, and can prevent similar bugs to happen. On the other hand it's a hack that does not explain why this browser is shown on the first place. I wish I had any better idea, but I'm really out of ideas.

Comment 13

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

Comment on attachment 8677489 [details] [diff] [review]
prevent the pre-loaded browser to be displayed. v1

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

So I _think_ I understand how this is happening.

Let's say we have two tabs open. Let's also say we have a preloaded about:newtab page. Our tabs should look like this:

 /------------------\ /--------------------\
/       Tab 1        \         Tab 2        \


And the tabpanels are, I think, a <xul:deck> stacked up something like this:

<tab 1 xul:browser>
<tab 2 xul:browser>
<preloaded about:newtab page>


When we close Tab 2, tabbrowser.xml's "removeTab" is called, which calls "_blurTab", and eventually kicks off a tab animation that closes Tab 2. Once the tab animation ends, tabbrowser.xml's "_endRemoveTab" method gets called.

"_blurTab" seems to be the method responsible for choosing the right tab after one closes. In this case, it'll choose Tab 1, as the only remaining tab, and set the selectedTab attribute to it.

Now here's the kicker - with e10s, setting the selectedTab is asynchronous, and doesn't actually do the tab switch until the frame is ready from content. Keep that in mind while we move on to the next step...

The tab animation finishes and "_endRemoveTab" is called - this method is responsible for actually removing the tab node and the associated browser. So Tab 2 is removed, and Tab 2's xul:browser is also removed.

Suppose now that the asynchronous switching work in selectedTab has not finished yet - like, we haven't gotten a frame yet. What happens?

Well, Tab 2's xul:browser has been removed and we're still waiting on that frame... and when you remove an element from a deck, I _believe_ what happens is you expose the next element in the deck. In this case, that'd be the about:newtab page.

So I _think_ that's why this is occurring.

I don't think setting the about:newtab page to the preloading state is the right solution - you're right, it's a workaround.

There are a few other solutions I can think of:

1) Make the preloaded about:newtab transparent or visibility: hidden until it is needed.
2) Somehow wait until the frame is ready from content before removing the original browser - basically, don't shift the deck until the child is ready.
3) Move the preloaded browser elsewhere so that it's not in the deck anymore, or it's not at the end of the deck anymore.

Our async tab switching stuff is kinda tragic. I'm really hoping we can come up with better, more robust solutions for fast and reliable tab switching in M9.

Comment 14

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> So I _think_ I understand how this is happening.

Thanks for the detailed description, this is a very likely scenario based on my logging.

> Now here's the kicker - with e10s, setting the selectedTab is asynchronous,
> and doesn't actually do the tab switch until the frame is ready from
> content. Keep that in mind while we move on to the next step...

Yeah, I've realized this. This part is really... interesting.

> frame... and when you remove an element from a deck, I _believe_ what
> happens is you expose the next element in the deck. In this case, that'd be
> the about:newtab page.

This is the piece of code I've been looking for for 2 days I think, could you
point me to where this might happen? I really could not find it.

> I don't think setting the about:newtab page to the preloading state is the
> right solution - you're right, it's a workaround.

Could you explain me why? Don't get me wrong it's kind of hacky, but the whole code in this area has right now more history than concepts and this is a very low risk fix compared to the other options. A serious refactor is needed in this area later, that we can agree on. But for now, I don't think it's such a bad idea.

> 
> There are a few other solutions I can think of:
> 
> 1) Make the preloaded about:newtab transparent or visibility: hidden until
> it is needed.

This was the first thing I tried, but if it's hidden it comes with a bit of delay when the preloaded tab is used and we 'unhide' it. So we lose some of the optimization that the preloaded browser gives us. I also tried to just flag the browser and then when the deck removal happens prevent somehow the preloaded browser to be selected. Since I could not find where this actually happening, I could not do this. I'm not entirely sure that this is happening any longer, but this was my theory as well. I just cannot find a proof of it in the code.

> 2) Somehow wait until the frame is ready from content before removing the
> original browser - basically, don't shift the deck until the child is ready.

I'm really scared to do this, I can imagine so many way this can go wrong :( But you understand this code a lot better than me, so if you say so I can give it a try. There is some double bookkeeping going on with some special getters/setters and that makes me feel uncomfortable to do any delay on one side...

> 3) Move the preloaded browser elsewhere so that it's not in the deck
> anymore, or it's not at the end of the deck anymore.
> 

I always wondered, why is it in the deck on the first place? I sort of guessed that there is a reason for it somewhere deep in XUL, and this is the only way to make the background loading to work... What would be the best place to move it to? Or do you mean, we should just move it from the deck in this special case temporarily?

> Our async tab switching stuff is kinda tragic. I'm really hoping we can come
> up with better, more robust solutions for fast and reliable tab switching in
> M9.

Yeah. Some of this stuff should be addressed on platform side. We should come up with a sane API that we can expose to JS and makes life a bit simpler here. The amount of juggling one have to make in JS to avoid the rendering engine to render something nonsense is quite scary.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

There's some code in nsDeckFrame.cpp that actually handles the tab switching at the lowest level. It might help debugging if you set breakpoints or add printfs there.

Comment 16

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #15)
> There's some code in nsDeckFrame.cpp that actually handles the tab switching
> at the lowest level. It might help debugging if you set breakpoints or add
> printfs there.

Thanks Bill, this is exactly what I was looking for: http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsDeckFrame.cpp#176. If I change this so, that we decrease mIndex when we remove the current frame (currentFrame != aOldFrame and ofc mIndex > 0) then the problem goes away. It's a pity that most of the cases decreasing the index is not what we want since we're supposed to switch to the next tab not the previous one. Oddly there is no side effect, but still this is not the right way to fix it.

What could work if we checked if the next frame belongs to the preloaded browser and if so only then we decrease mIndex. Is there a way to check that? I'm not sure how can we look up the browser that an IFrame belongs too. If we could do that I could just check GetNextBox.

Comment 17

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> (In reply to Bill McCloskey (:billm) from comment #15)
> > There's some code in nsDeckFrame.cpp that actually handles the tab switching
> > at the lowest level. It might help debugging if you set breakpoints or add
> > printfs there.
> 
> Thanks Bill, this is exactly what I was looking for:
> http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsDeckFrame.cpp#176.
> If I change this so, that we decrease mIndex when we remove the current
> frame (currentFrame != aOldFrame and ofc mIndex > 0) then the problem goes
> away. It's a pity that most of the cases decreasing the index is not what we
> want since we're supposed to switch to the next tab not the previous one.
> Oddly there is no side effect, but still this is not the right way to fix it.
> 

I think I wrote this code, and it had to do with some hackery that was going on in the browser code, and I thought this would fix it properly.

Context: https://bugzilla.mozilla.org/show_bug.cgi?id=1009628#c24

> What could work if we checked if the next frame belongs to the preloaded
> browser and if so only then we decrease mIndex. Is there a way to check
> that? I'm not sure how can we look up the browser that an IFrame belongs
> too. If we could do that I could just check GetNextBox.

We'd want to make sure this doesn't re-open the issue I refer to above.

Comment 18

[Gabor Krizsanits (INACTIVE)]

Attachment: prevent the pre-loaded browser to be displayed. v2

Finally I've got some time to get back to this one... I think delaying the panel removal makes the most sense. It might help to implement what Bill wanted to do with the original problem. (I have not got any luck there yet but this step should be a pre-requirement for it)

Comment 19

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

Comment on attachment 8683160 [details] [diff] [review]
prevent the pre-loaded browser to be displayed. v2

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

::: browser/base/content/tabbrowser.xml
@@ +2379,5 @@
> +              // We don't want to remove the tabPanel just yet. If the subsequent panel
> +              // in the deck is the preloaded browser for the new tab, by removing it
> +              // now would show the preloaded browser for split second, so let's wait
> +              // with it a bit longer.
> +              if (this.tabPanelToRemove) {

This worries me. I'm pretty sure the TabSwitcher isn't used for single-process Firefox, so I think what this will result in (in that case), is browsers never being removed from the DOM.

So if we're going to go this route, we should make sure that we only stash the tabpanel if we're using async tab switching.

Comment 20

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

Hey billm, do you still actually see the newtab flash thing?

I just tried to reproduce the bug, and when the content process is blocked up with a while(true){}, it looks like the tab won't let me close it. I can switch tabs (and get the spinner), but no closing, which means that I don't end up seeing the preloaded browser sitting in the back.

So do you still see this?

Comment 21

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #20)
> Hey billm, do you still actually see the newtab flash thing?

I cannot see it any longer on the latest nightly on OSX. (just the spinner)

> 
> I just tried to reproduce the bug, and when the content process is blocked
> up with a while(true){}, it looks like the tab won't let me close it. I can
> switch tabs (and get the spinner), but no closing, which means that I don't
> end up seeing the preloaded browser sitting in the back.

I have to click 2-3 times in a row for closing it.

Comment 22

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #21)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #20)
> > Hey billm, do you still actually see the newtab flash thing?
> 
> I cannot see it any longer on the latest nightly on OSX. (just the spinner)
> 
> > 
> > I just tried to reproduce the bug, and when the content process is blocked
> > up with a while(true){}, it looks like the tab won't let me close it. I can
> > switch tabs (and get the spinner), but no closing, which means that I don't
> > end up seeing the preloaded browser sitting in the back.
> 
> I have to click 2-3 times in a row for closing it.

Ah, yep, multiple clicks will do it.

Still no newtab flash though. I just tried on my OS X machine and my Ubuntu VM, and no dice. Let's see if billm still sees it.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't see it anymore. We didn't used to need multiple clicks though.

Unfortunately, tab closing still looks bad during normal usage for me. I guess I'll need to look for a new STR to torment someone with :-).

Comment 24

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

Alright. I hope it's cool that I leave this needinfo on you then for those STR.

Comment 25

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

Gentle ping. billm, I still don't know how to reproduce this one, and it's my last M8.

Comment 26

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

I talked to billm about this on Monday. He's no longer seeing the newtab flicker.

He is seeing a black flash when switching tabs, however.

This bug has really kinda meandered off its original intent. I'm going to file a new bug for the black flash that billm is seeing and close this WFM.