Weird code in restoreTabs

RESOLVED FIXED in Firefox 35

Status

()

Firefox
Session Restore
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: billm, Assigned: ttaubert)

Tracking

Trunk
Firefox 35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
I was looking over the code in restoreTabs and I noticed something that seems worrisome:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2434

This code seems to assume that aTabData is for all the tabs in the window. However, we frequently call restoreTabs for only one tab. undoTabClose does this for example.

I couldn't actually make this lose session data, but maybe I just didn't try hard enough. Either way the code seems wrong. Tim, do you know what's going on here?
Flags: needinfo?(ttaubert)
(Assignee)

Comment 1

3 years ago
I added that in bug 919532, and yeah that doesn't look right - good catch! This will probably make us lose data if we undo-close a tab and then quickly shut down without notifying before shutting down. We hopefully don't have any code that does that though.
Flags: needinfo?(ttaubert)
(Assignee)

Comment 2

3 years ago
Created attachment 8489914 [details] [diff] [review]
0001-Bug-1067648-Introduce-restoreTab-and-use-it-from-res.patch

I created another method restoreTab() that properly handles single tabs and is called by restoreTabs(). That at the same time is the method that duplicateTab(), setTabState(), and undoCloseTab() call now.

As the window's busy state was handled by restoreTabs() before we now need something that can handle single restoreTab() calls and restoreTab() being called in a loop - so I added a busy state counter.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8489914 - Flags: review?(wmccloskey)
(Reporter)

Comment 3

3 years ago
Comment on attachment 8489914 [details] [diff] [review]
0001-Bug-1067648-Introduce-restoreTab-and-use-it-from-res.patch

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

I'm afraid I no longer remember what the busy state is for. But why do you need to add a counter for it? It seems like it will never go higher than 1 since it's increased at the top of restoreTab and decreased at the bottom. And, as far as I can tell, restoreTab can't recurse into itself.

::: browser/components/sessionstore/SessionStore.jsm
@@ +2395,5 @@
>      tabstrip.smoothScroll = smoothScroll;
>  
>      TelemetryStopwatch.finish("FX_SESSION_RESTORE_RESTORE_WINDOW_MS");
>  
> +    this._setWindowStateReady(aWindow);

What's this for?

@@ +2415,2 @@
>     */
> +  restoreTabs: function (aWindow, aTabs, aTabData, aSelectTab) {

Do we need to pass in aTabs anymore? For this function to work, we need aTabs to be all the tabs in aWindow, so maybe we should just do that.
(Assignee)

Comment 4

3 years ago
Created attachment 8491490 [details] [diff] [review]
0001-Bug-1067648-Introduce-restoreTab-and-use-it-from-res.patch, v2

(In reply to Bill McCloskey (:billm) from comment #3)
> I'm afraid I no longer remember what the busy state is for. But why do you
> need to add a counter for it? It seems like it will never go higher than 1
> since it's increased at the top of restoreTab and decreased at the bottom.
> And, as far as I can tell, restoreTab can't recurse into itself.

The busy state tells add-ons when they should ignore certain basic operations like opening and closing tabs because sessionstore is restoring a whole window and they should rather wait until that's done.

I added the counter because otherwise we would be sending busy/ready events after each call to restoreTab() instead of only once per window. To actually make that work we still have the busy/ready state changes at the beginning and end of restoreTabs() that wrap all of the restoreTab() calls.

restoreTab() contains busy/ready state changes itself because they're needed for undoCloseTab(), setTabState(), and duplicateTab(). They call restoreTab() now instead of restoreTabs().

> > +  restoreTabs: function (aWindow, aTabs, aTabData, aSelectTab) {
> 
> Do we need to pass in aTabs anymore? For this function to work, we need
> aTabs to be all the tabs in aWindow, so maybe we should just do that.

We actually still need to pass the tabs itself because that might be a subset of a window's tabs when we don't override an existing state but "append". Fixed the method to not throw away state for tabs that aren't overriden.
Attachment #8489914 - Attachment is obsolete: true
Attachment #8489914 - Flags: review?(wmccloskey)
Attachment #8491490 - Flags: review?(wmccloskey)
(Reporter)

Comment 5

3 years ago
Comment on attachment 8491490 [details] [diff] [review]
0001-Bug-1067648-Introduce-restoreTab-and-use-it-from-res.patch, v2

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

Ah, okay. I think the crucial thing I missed is that we call setWindowStateBusy at the top of restoreWindow. Would it be possible to move that call (and the corresponding setWindowStateReady call) into restoreTabs?
Attachment #8491490 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 6

3 years ago
(In reply to Bill McCloskey (:billm) from comment #5)
> Ah, okay. I think the crucial thing I missed is that we call
> setWindowStateBusy at the top of restoreWindow. Would it be possible to move
> that call (and the corresponding setWindowStateReady call) into restoreTabs?

We actually need to keep those in restoreWindow() as that opens new windows, unpins tabs, and moves tabs around.
(Assignee)

Updated

3 years ago
OS: Linux → All
Hardware: x86_64 → All
Version: 34 Branch → Trunk
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/8b9340f3a185
https://hg.mozilla.org/mozilla-central/rev/8b9340f3a185
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.