Session store should provide a method for clearing out current tabs via setBrowserState

NEW
Unassigned

Status

()

Firefox
Session Restore
--
minor
9 years ago
2 years ago

People

(Reporter: whimboo, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

9 years ago
+++ This bug was initially created as a clone of Bug #476928 +++

With bug 476928 fixed we have mostly a seamless change into private browsing mode. But having a tab at the end of the tabbar open still shows the first tab before the private browsing mode is entered. That ends up in kinda short flickering.

Given by bug 476928 comment 19 we should try to investigate if we can make it even more smoother.
The tab you're seeing is probably the transistionState tab, which is necessary due to bug 476463, however I think there may be a way to flush the events on any given document, which would make this much smoother, cc'ing Boriss to see if there is such a thing.

Updated

9 years ago
Keywords: verified1.9.1
(Reporter)

Updated

9 years ago
Summary: Slow closing of tabs with the given testcase when entering Private Browsing mode → Further improvements for a seamless switch to Private Browsing mode
I think you mean bz.  Boriss is a UX designer (Jennifer Boriss). :)
I'm not sure what comment 1 is asking, exactly....  Is the problem that the new tab is visible to the user and then we close it?  Is what's desired here just locking down painting for all toplevel windows until after the transition is complete or some such?
Boris: Here is a description of what I see using the 1.9.1 B3 versus the trunk, which I think henrik is explaining.

Using 1.9.1 B3, if I have a set of tabs open and launch PB mode, it chunks the tabs closed one at a time (I can see each one for a moment before they close, and the same thing happens when I exit PB mode and go back to regular mode)

Using the trunk, if I have a set of tabs open and launch PB mode, it switches seamlessly to the mode without seeing all the open tabs close one at a time.
Oh.  I have no idea why that difference would be there, offhand.
I think what Nochum meant was asking if there is any way to force an unload event on the set of currently open documents without closing down the tabs they're residing in, so that we can avoid opening an about:blank page just for the sake of getting the onload event reliably fired on the document.
Sure.  For example, you could navigate all those documents to about:blank...
Yes, that's what we do now.  Is there any other way to do this, preferably something which doesn't cause a repaint?
(Reporter)

Comment 9

9 years ago
(In reply to comment #4)
> Using 1.9.1 B3, if I have a set of tabs open and launch PB mode, it chunks the
> tabs closed one at a time (I can see each one for a moment before they close,
> and the same thing happens when I exit PB mode and go back to regular mode)

Something has been changed here in the last two days. I cannot see see the behavior I've subscribed in comment 0 anymore. Instead we have the old behavior before the patch on bug 476928 was checked-in. I've removed the verified1.9.1 keyword there and we should wait with this bug until bug 476928 is really fixed on 1.9.1.
If we have a regression, I'd love a regression range, of course!
Ehsan is right, just to expand a little:

We were forced to have a transition state between regular browsing and pb and vice-versa because of cookies (eg) set onunload of a page would persist to the next mode. Same goes for exceptions in the error console.

To mitigate this, we set the browser state to two blank tabs, manually call removeTab on the first one, then move to the next state. This successfully ensures that no private data leaks into the next mode.

I was asking if we could just switch modes, then call some method to flush any events left on the documents. I was thinking of FlushPendingEvents but wasn't sure what it does (if it is what we want) and if it can be called from js.
Unload events are synchronous (modulo alerts from beforeunload and the like), so I'm not sure why there's this much complexity here to start with...
Session Restore is async though afaik. We were reusing the tabs (thats how session store works) and that's where the problem was.
(Reporter)

Comment 14

9 years ago
It looks like that the patch on bug 476928 doesn't have fixed it at all while on trunk it is fine and gives us a seamless switch over.

Comment 15

9 years ago
I've got a simple patch to fix most of this issue in SessionStore over in bug 476928 (failed to note this bug and could move it over if desired).
(Reporter)

Comment 16

9 years ago
No, it's fine Simon. We could still use this bug afterward if we need some more investigation. Thanks!
Is there anything left to do in this bug or has Simon's patch resolved this?

bz: if I reuse a tab and load a new url am I guaranteed that by the time the net piece of code runs onunload will have fired on the first document? Tab a has google.com loaded, I switch it to about:blank then run some unrelated code afterwards, am I guaranteed that google.com receive its unload event by the time the code runs (even if I didn't close the tab)?
> Tab a has google.com loaded, I switch it to about:blank then run some unrelated
> code afterwards, am I guaranteed that google.com receive its unload event by the
> time the code runs

Not at all.  In fact, you're guaranteed the opposite.  The unload will happen asynchronously, when the about:blank load starts handing back data.
(Reporter)

Comment 19

9 years ago
Simons new patch hasn't been checked into 1.9.1 yet. We are still waiting for approval. We have to wait until I can verify it with a nightly build.
(In reply to comment #18)
...which is why the tab add/remove hack is necessary. I guess we can use this bug to implement it in sessionstore, although that involves a bit of morphing :)
(Reporter)

Comment 21

9 years ago
Now, with bug 476928 fixed on 1.9.1 it feels much better and I don't have something to add to this bug. Nochum, if you have ideas for further improvements feel free to morph the bug a bit.
(In reply to comment #21)
> Now, with bug 476928 fixed on 1.9.1 it feels much better and I don't have
> something to add to this bug. Nochum, if you have ideas for further
> improvements feel free to morph the bug a bit.

Do you mean that the speed matches that of trunk builds, or we still need further improvements?
(Reporter)

Comment 23

9 years ago
Yes, that's why I have verified bug 476928 on 1.9.1. But see comment 20 which sounds like more improvements.
(In reply to comment #23)
> Yes, that's why I have verified bug 476928 on 1.9.1. But see comment 20 which
> sounds like more improvements.

Nochum, would you like to morph this bug according to comment 20?
morphed.
Component: Tabbed Browser → Session Restore
QA Contact: tabbed.browser → session.restore
Summary: Further improvements for a seamless switch to Private Browsing mode → Session store should provide a method for clearing out current tabs via setBrowserState
Version: 3.1 Branch → Trunk
Simon: does session store want this fixed? It sounds like it's something specific to private browsing, as the session won't be restored synchronously even with this method. The only advantage is that the current tabs will be removed synchronously, then the new state will be loaded asynchronously. This would be a messy api and I can't really understand why it fits in session store...

Thoughts?

Comment 27

9 years ago
(In reply to comment #26)
I'd actually not expect e.g. cookies being set by setBrowserState being overwritten by an onunload handler. So yes, we do want the issue from your comment #11 to be fixed inside SessionStore.
This is needed for bug 498648, patch will be there.
Assignee: nobody → highmind63
Depends on: 498648
No longer depends on: 476928
Bug 498648 was fixed without this, I'll look into this for .next anyhow.
No longer depends on: 498648
->default
Assignee: highmind63 → nobody

Updated

2 years ago
Severity: normal → minor
You need to log in before you can comment on or make changes to this bug.