651033 - Registering nsISHistoryListener seems to cause Session Store to lose its own listener

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Daniel Dawson]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0

I'm developing an extension that tracks session history through nsISHistoryListeners so it can build a tree of the navigations in each tab. For some reason, it seems that registering my own nsISHistoryListeners at the right point during session restore is causing the Session Store service to lose its own listeners (or at least that's my guess as to what the error message means, since I encountered it on my own code at one point).

The symptoms of this bug (that only the number of tabs specified in browser.sessionstore.max_concurrent_tabs are loaded and the rest stay blank) were first reported in bug 648856. Although that was resolved invalid due to my extension being isolated as a proximal cause, further investigation on my part strongly suggests that the underlying cause really is in Mozilla code. Specifically, I reduced my extension to a near-minimum required to trigger the behavior, and I can't see anything really wrong with it.

Error: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHistory.removeSHistoryListener]
Source file: resource://gre/components/nsSessionStore.js
Line: 3957

I have the test case prepared, but I'm not sure in what form you want it. Would a XPI uploaded here be okay? I'll go ahead and upload it, but let me know if you want something else.

I will admit that I'm not 100% sure this isn't really something I'm doing wrong, though it's hard to imagine that simply registering my own listener would properly cause such an issue.

Reproducible: Always

Steps to Reproduce:
1. Install my reduced extension (to be uploaded, or something).
2. Load a session with more tabs than the value of browser.sessionstore.max_concurrent_tabs. Having it load at startup by configuring "Show my windows and tabs from last time" works, at least.

Actual Results:  
The following error appears in the console (using Console²) some time after SSTabRestored events fire (the relevant registration of my handlers happens in between):

Error: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHistory.removeSHistoryListener]
Source file: resource://gre/components/nsSessionStore.js
Line: 3957

As a result of the error, all of the tabs that weren't already being loaded never do load.

Expected Results:  
Registering an nsISHistoryListener should never cause others to disappear, assuming that's what's happening.

Comment 1

[Daniel Dawson]

Attachment: Small extension that triggers the described behavior

Comment 2

[Daniel Dawson]

Also, although I've marked this as 4.0, I also observed it with 6.0a1.

Comment 3

[u279076]

Paul, can you provide any expert feedback on this bug?

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I just investigated and the reason this is breaking is because when session store goes to remove it's listener, an error is thrown.

Apparently there can only be one listener at a time... AddSHistoryListener pushes out whatever was there (in this case session restore's listener is being replaced by the addon's).

This seems like a really broken API (there's no mention of only allowing a single listener)... Olli, perhaps you can spread some light here? It seems like it should allow multiple listeners and check each one.

Regardless session restore should probably try+catch that. The listener was added to make sure reloading worked (bug 597757), but if the listener is replaced, we're out of luck...

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Fixed by bug 422543.