1371884 - Instead of replacing initial tab on session restore, overwrite it (essentially, revert bug 1054740)

Status: RESOLVED DUPLICATE of bug 1365541

(Firefox :: Session Restore, defect)

Description

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

In bug 1054740, we were attempting to fix a problem where about:home was being displayed for some time in the initial window before being overwritten with the previous session during automated session restore.

The way we solved this was by replacing the initial tab with the new tabs for the session instead of what we were going before, which was overwriting it.

Hindsight being 20/20, I think this solution was a mistake, for the following reasons:

1) It didn't actually solve the problem in some cases. See bug 1362755.
2) There was a slew of fallout. See bug 1362866 (and _its_ fallout), bug 
1365541, and bug 1365541.
3) I think this makes us do _more_ work in the parent process than the original overwriting approach.

For (3), I think this is because when we replace the tab, we:

a) Tear down the initial browser (which in the e10s case, is probably remote, which means probably starting the shutdown of a content process)
b) Create a new browser to replace it, which means possibly initializing a new content process.

I think that has the potentially to be quite a bit more expensive than what we were dealing with before, which was a sometimes about:home load in a content process. Bug 1365541 seems to suggest that this might be happening in the wild.

That's not to say that good things didn't come out of bug 1054740. We ended up simplifying the SessionStore.jsm code, and getting rid of a lot of focus changes in bug 1362866, which was nice.

Here's what I suggest:

1) Re-jig SessionStore.jsm so that we overwrite the initial browser again instead of replacing it
2) Ensure that we continue to move that initial browser to the right selected index during session restoration
3) Replace the blanking-out logic introduced in bug 1362866 with something more robust. Namely, have the TabChild tell the TabParent when it's _possible_ to paint (a TabChild exists, something paintable has finished loading), and expose that as a bool for checking blank-ability, instead of hasPresented. This should help address the remaining instances of bug 1367596.

Again, this is unfortunately happening late in the cycle. We'll probably end up shipping bug 1054740 to release in 55. For my part, I should have pushed back harder in bug 1362866 and gotten bug 1054740 backed out earlier in the cycle, but I didn't, and that's my fault.

Dao, do you concur with the above?

Comment 1

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

ni?ing for feedback on comment 0.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #0)
> 1) It didn't actually solve the problem in some cases. See bug 1362755.
> 2) There was a slew of fallout. See bug 1362866 (and _its_ fallout), bug 
> 1365541, and bug 1365541.
> 3) I think this makes us do _more_ work in the parent process than the
> original overwriting approach.

(1) was known when I landed the patch, it doesn't make it a mistake. It's a partial solution.

Bug 1365541 and (3) are the same, I think.

> For (3), I think this is because when we replace the tab, we:
> 
> a) Tear down the initial browser (which in the e10s case, is probably
> remote, which means probably starting the shutdown of a content process)
> b) Create a new browser to replace it, which means possibly initializing a
> new content process.

Gabor, do you think bug 1364849 will help here?

Comment 3

[Gabor Krizsanits (INACTIVE)]

(In reply to Dão Gottwald [::dao] from comment #2)
> > For (3), I think this is because when we replace the tab, we:
> > 
> > a) Tear down the initial browser (which in the e10s case, is probably
> > remote, which means probably starting the shutdown of a content process)
> > b) Create a new browser to replace it, which means possibly initializing a
> > new content process.
> 
> Gabor, do you think bug 1364849 will help here?

Short answer: yes, that patch is intended to solve exactly this kind of issues. To be precise, there is a slot for a cached content process in the preallocated process manager. If it's not taken yet, then after (a) we pick it up and in (b) we reuse it, so no process is created or destroyed. If the slot was already taken (so we have a preallocated process at hand) then (a) will destroy a process and (b) will reuse the cached process we have (and then later at some point we will startup a new preallocated process).

So bug 1364849 will help but won't be a perfect solution here. We can tune it if needed (we could add a second slot that only keeps alive a second cached process for a short while...), I was more worried about the cases where we end up mass creating and destroying processes within a short time.

Comment 4

[Dão Gottwald [:dao]]

I re-implement reusing the selected tab in bug 1365541 since bug 1364849 doesn't seem to have fixed the regression.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #0)
> 3) Replace the blanking-out logic introduced in bug 1362866 with something
> more robust. Namely, have the TabChild tell the TabParent when it's
> _possible_ to paint (a TabChild exists, something paintable has finished
> loading), and expose that as a bool for checking blank-ability, instead of
> hasPresented. This should help address the remaining instances of bug
> 1367596.

Do you still want to do this? Was this on file somewhere other than as part of this bug?

Comment 6

[randomreporter]

(In reply to Dão Gottwald [::dao] from comment #5)
> Do you still want to do this? Was this on file somewhere other than as part
> of this bug?

I guess your comment is related to the problem I mentioned in the bug 1365541: white flashes when switching to a loading tab so I'll post updated STR here:

Enable bookmarks toolbar, create https://videocardz.com/ bookmark on it
Drag this bookmark to the tab bar in order to open it in the background
Count to 10 in order to make sure contents of the web page are rendered, videocardz tab should still be loading
Switch to this tab

ER: Contents of the web page are rendered immediately
AR: Contents of the web page are rendered after annoying white flash

If videocardz stops loading before you count to 10 then use this link instead:
http://www.rbc.ru/politics/30/04/2017/590639d09a79471c8e410316

Comment 7

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

(In reply to Dão Gottwald [::dao] from comment #5)
> Do you still want to do this? Was this on file somewhere other than as part
> of this bug?

Yes I do. I just filed bug 1378203 for this.