1217026 - Firefox Nightly sometimes fails to remove (some) closed windows from sessionstore.js

Status: NEW

(Firefox :: Session Restore, defect)

Description

[Chris Siebenmann]

I have my Firefox set to 'When Firefox starts: Show my windows and tabs
from last time' (with a number of persistent Firefox windows that I
basically always have around). For around the last month or so, Firefox
Nightly has been leaving some closed windows in my sessionstore.js,
so that if I quit and restart they'll come back. Such 'leaked' windows
can be blank windows or windows for random sites, and the number of
them can vastly exceed my real, still-open windows from when I shut
down Firefox Nightly. Having them all come back when I quit and restart
Firefox is more than a bit of a problem, especially because they bloat
Firefox's memory usage. This problem persists over things like closing
all windows one by one, removing all sessionstore files and directories,
and restarting Firefox. The newly restarted Firefox is clean, but if I
use it for a while and then close it, some windows I closed during the
session come back.

Not all windows that I close linger on in sessionstore.js; most of
them are removed. And I'm familiar with how sessionstore.js can lag
somewhat behind Firefox's actual state (including at shutdown) and so
restore a few of your very recently closed windows; this is not that
behavior. When windows are leaking, I can close Firefox, restart, get a
blizzard of were-closed windows, close them all again, use Firefox for
hours, shut it down again, start it up again, and have many of those
closed-at-startup windows come back.

By watching the browser console for errors while I was closing windows,
I have seen three exceptions that seem session store related (although
at least one may be a red herring):

- TypeError: win is null SessionStore.jsm:644:9

  This is in receiveMessage(), at the line:
	let tab = win.gBrowser.getTabForBrowser(browser);

- TypeError: listener is not a non-null object RemoteAddonsParent.jsm:608:3

  This is in makeFilteringListener(), at the line:
	filteringListeners.set(listener, filter);

- SecurityError: The operation is insecure. SessionStorage.jsm:147:0

  This is in _readEntry, at the line:
	if (storage && storage.length) {

  The interior for loop has a try/catch around the access to storage.key
  with a comment about secured items, but apparently this is not
  sufficient to avoid problems in some circumstances.

I built a patched version of Firefox Nightly that crudely avoids all
three of these exceptions and it does not appear to have problems with
'leaking' closed windows into sessionstore.js / recover.js / etc.
Avoiding only the first or just the first two exceptions is not
sufficient to avoid the leaks, but I have not tested other combinations.

(In receiveMessage(), I guard actually assigning tab a value with a
'if (win) ...'; in makeFilteringListener(), I added 'if (!listener)
{return listener;}' at the start; in _readEntry() I put the entire if
in another try/catch block. I'm mentioning my changes in case they have
other side effects that I'm not aware of. I can put my actual diffs here
if it would help.)

I'm using Firefox Nightly in non-e10s mode, I have a number of addons
installed, and I'm on 64-bit Linux (Fedora 22). Also, I run the same
Firefox Nightly setup on another almost-identical machine and it doesn't
seem to have the problem so far.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1217026 - Making Session Restore more resilient to some exceptions;r?ttaubert

Bug 1217026 - Making Session Restore more resilient to some exceptions;r?ttaubert

Comment 2

[David Teller [:Yoric] - still alive but not very active]

It is the first time I hear about these errors, so they may be caused by an add-on. Regardless, making Session Restore more bulletproof is always a good idea. I haven't touched RemoteAddonsParent, because it's very unrelated code.

Comment 3

[Chris Siebenmann]

I suspect that the SessionStore.jsm change isn't the right one. Comments
later on in receiveMessage() seem to imply that the tab may have been
destroyed by now, and this may extend to the window itself, eg the
'SessionStore:update' case. Simply exiting receiveMessage() will not
fall through to handling this. Having no tab is explicitly okay for
SessionStore:update (per NOTAB_MESSAGES).

It may be significant for seeing this problem that I mostly use windows
with single tabs, not multiple tabs in a single window. This may not be
a widely tested case.

(If 'win' in the code is something representing the overall window,
might closing the last tab in a window tear down the window fast enough
that receiveMessage() is not run before this happens and so the window
could disappear just as the code comments imply the tab can?)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8677005 [details]
MozReview Request: Bug 1217026 - Making Session Restore more resilient to some exceptions;r?ttaubert

https://reviewboard.mozilla.org/r/22851/#review20991

::: browser/components/sessionstore/SessionStorage.jsm:153
(Diff revision 1)
> -        } catch (e) {
> +          } catch (e) {
> -          // This currently throws for secured items (cf. bug 442048).
> +            // This currently throws for secured items (cf. bug 442048).
> -        }
> +          }
> -      }
> +        }
> -    }
> +      }
> +    } catch (e) {
> +      // `storage.length` can throw for secured items (cf. bug 1217026).
> +    }

I think we can remove the inner try/catch if all is wrapped.

::: browser/components/sessionstore/SessionStore.jsm:615
(Diff revision 1)
> +    if (!win) {
> +      // The document has no browsing context. It's not clear exactly when
> +      // this can happen.
> +      return;
> +    }

Probably when a window goes away and the message takes some time to deliver, not sure. Doesn't hurt to check indeed.

Comment 5

[Wayne Mery (:wsmwk)]

whic addons are  you using?

Comment 6

[Chris Siebenmann]

My current addons are:

- CS Lite Mod
- FireGestures
- FlashStopper
- HTTPS-Everywhere (EFF's)
- It's All Text
- NoScript
- Open in Browser
- uBlock Origin

None of them are recent additions; this collection has been stable for
me for a fairly long time (well before this issue started appearing).

Comment 7

[Elhem Enohpi]

Chris, a couple of questions:

- are there specific web page URLs that refuse to stay closed?
- do you have cookies disabled for them?
- does disabling javascript fix the problem?

Please see bug 1238178.

Comment 8

[Chris Siebenmann]

Which web pages failed to close were basically random as far as I could
tell, but I believe it sometimes included both completely blank pages
and my start page, which is a static HTML file on my disk (and has no
embedded JS).

I have both cookies and JavaScript blocked for almost everything. JS is
blocked via NoScript; cookies are blocked via a filtering proxy (basically
Privoxy) for HTTP sites (obviously it can't work on HTTPS sites) and
CS Lite Mod in Firefox for all sites. Checking, I also have this set to
off in Firefox (which may be the work of CS Lite Mod, I forget).

So I don't believe this is your bug 1238178.

(And at least parts of a fix for this landed in Nightly with the commit
to close bug 1171708.)

Comment 9

[Elhem Enohpi]

Thanks Chris.

I've noticed that re-opened windows may display the Start or New Tab page, if that's what was immediately prior in the history.

However, if this is happening with Javascript disabled, that's somewhat different from what I'm experiencing.

Comment 10

[Elhem Enohpi]

I've created a simple web page that might be helpful in testing.

With cookies disabled, it consistently produces "SecurityError: The operation is insecure. SessionStorage.jsm", when accessing sessionStorage on page load. If a window displaying this page is closed, Firefox 34-36 fails to remove it from sessionstore.js, and it will be re-opened on restore. It will show the previous page in the tab's history, which may be the Start or New Tab page.

https://d.maxfile.ro/rbbidjxuiv.html

The following page runs the same code on a button click rather than on page load. It behaves normally.

https://d.maxfile.ro/wkxfsryicj.html

Comment 11

[Elhem Enohpi]

Oops, comment 10 should say "Firefox 43-46"...