402349 - "aTabData.entries.forEach is not a function" error in nsSessionStore.js on call to getBrowserState function

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Michael Kraft [:morac]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9

While I haven't had any problems, a number of people have been filing bug reports on the Session Manager extension stating that when attempting to save a session.  The reports all contain the following error:

JavaScript Error:"aTabData.entries.forEach is not a function" {file: "file:///usr/lib/firefox/components/nsSessionStore.js" line: 1043}]' when calling method: [nslSessionStore::getBrowserState]

Session Manager calls the getBrowserState API function to get the browser state.  This function should not cause an error.  Since the reports started coming in after Firefox 2.0.0.8 was released and I haven't made any changes to the code I'm assuming this is something caused by a change in nsSessionStore.js

Reproducible: Sometimes

Steps to Reproduce:
1.  Call nsSessionStore's getBrowserState() API call

Actual Results:  
JavaScript Error:"aTabData.entries.forEach is not a function" {file: "file:///usr/lib/firefox/components/nsSessionStore.js" line: 1043}]' when calling method: [nslSessionStore::getBrowserState]

Expected Results:  
Should return state data

I am unable to reproduce this problem, but since the bug reports started coming in after Firefox 2.0.0.8 was released and I haven't made any changes to Session Manager since then I'm assuming this is a Firefox bug and filing it on behalf of the following people based on their bug reports at:

https://www.mozdev.org/bugs/show_bug.cgi?id=18028
https://www.mozdev.org/bugs/show_bug.cgi?id=18036
https://www.mozdev.org/bugs/show_bug.cgi?id=18039

Comment 1

[Michael Kraft [:morac]]

I've managed to reproduce this problem myself, though it isn't 100% reproducible.  It appears to occur when the SessionStore API is used to save a session just as a session finishes loading.

I've also gotten the browser to crash by doing the above.
Talkback Incident IDs:
TB38560508M
TB38560632H

I believe the problem was introduced by bug 367605.  Specifically I believe the problem is caused by line 1375 in nsSessionStore.js added by the patch for bug 367605.  This allows the entries variable for an aTab to be null.
http://mxr.mozilla.org/mozilla/source/browser/components/sessionstore/src/nsSessionStore.js#1375

The problem occurs when line 1101 gets hit before the entries variable is populated.  
http://mxr.mozilla.org/mozilla/source/browser/components/sessionstore/src/nsSessionStore.js#1101

This can take a few seconds after a session loads.  If the line is hit before then an exception will be thrown.

A relatively simple fix is to check to see if aTabs.entries is an array (it exists) before attempting to doing a forEach on it.


The easiest way to reproduce this problem is to install the Session Manager extension from https://addons.mozilla.org/en-US/firefox/addon/2324 and the load the session I'm going to attach.  When the session appears to have loaded (windows stop popping around), go into the session manager and load the "[Last Replaced Browsing Session]".  This will save the current browser session and load the last one.  You might have to repeat this 2 or 3 times, but you will eventually get the exception above.

I'm attaching a branch and trunk patch which works around this.  You'' lose the hosts data for tabs when you save, but at least the session will save without throwing an exception.

If you can think of a better way of patching, then go for it.

Comment 2

[Michael Kraft [:morac]]

Attachment: Test Session

Steps to recreate:

1. Load this session, wait for it to appear to be finished and then load the last session.  
2. Load the last session (blank or homepage)
3. Load the last session again and wait for it appear to be finished.
4. Repeat steps 2 and 3 until you experience the exception

Comment 3

[Michael Kraft [:morac]]

Attachment: Branch patch for Firefox 2.x

This patch will prevent the exception, though the tab's host data will be lost.

Comment 4

[Michael Kraft [:morac]]

Attachment: Trunk patch

Trunk patch for Firefox 3 beta

Comment 5

[Simon Bünzli]

(In reply to comment #1)
> A relatively simple fix is to check to see if aTabs.entries is an array (it
> exists) before attempting to doing a forEach on it.

This won't instanceof Array for tabs which haven't completely loaded, but should nonetheless have a working forEach on its prototype. Is testing for the existence of aTabs.entries alone not enough (i.e. just doing |if (aTabs.entries) ...|)?

BTW: When you ask for a review, you'll have to set the review flag to a ? and then add the email address of the person you want to ask for review (e.g. Dietrich, Gavin or myself).

Comment 6

[Michael Kraft [:morac]]

(In reply to comment #5)
> This won't instanceof Array for tabs which haven't completely loaded, but
> should nonetheless have a working forEach on its prototype. Is testing for the
> existence of aTabs.entries alone not enough (i.e. just doing |if
> (aTabs.entries) ...|)?
> 

I tried originally tries testing for (aTabs.entries), but was still able to reproduce the problem. 

I just ran a test that I should have done in the first place and did a dump(aTabs.entries.toSource()) right before the forEach call to see why it was occurring.

This is what I saw when the error hit:

({0:{url:"file:///D:/Apps/voting/votingsystemtoolbox/vote-0-4/documentation/refe
rence.html", children:{}, title:"Voting Systems Toolbox - Instructions", ID:1186
922149, scroll:"0,0"}})

This looks identical to the issue that was corrected by bug 350558.  I had originally never considered that since I put a work around for bug 350558 in the Session Manager extension since it isn't in Firefox 2.x, but the problem is that because of a chance for bug 367605, that the issue is now being hit in SessionStore.js before my extension can correct the data.

Sure enough I could not reproduce this problem on the Trunk and when I implemented the fix for bug 350558 in Firefox 2.0.0.10, I couldn't reproduce the problem either.

I really should have did this in the beginning, but since I couldn't reproduce the problem until yesterday and it didn't start occurring until Firefox 2.0.0.8, it completely slipped my mind to try it on the Trunk.  Like I said I should have done the dump to begin with.

I guess this can be considered a duplicate of bug 350558, but since it is now affecting more people as of Firefox 2.0.0.8, is it possible to get bug 350558 fixed in the Firefox 2 branch?  Otherwise Session Manager won't work for many people until Firefox 3 is released (and they upgrade).

Comment 7

[Michael Kraft [:morac]]

Comment on attachment 290579 [details] [diff] [review]
Branch patch for Firefox 2.x

not needed

Comment 8

[Michael Kraft [:morac]]

Comment on attachment 290581 [details] [diff] [review]
Trunk patch

not needed

Comment 9

[Michael Kraft [:morac]]

Attachment: A sessionstore.js file that causes a loading failure which results in the problem documented here

I'm attaching a sessionstore.js file which fails to load correctly, in both Firefox 2.0 and the latest nightly beta of Firefox 3.0, when the restore tabs and windows from last time option in Firefox is selected.  This occurs regardless if Session Manager is installed or not.  

This results in the aTabData.entries.forEach error being thrown when trying to call the nsSessionStore.getBrowserState() function in both Firefox 2.0 and the latest nightly beta of Firefox 3.0.

The session data was corrupted by bug 350558 so you shouldn't end up with sessions like this in Firefox 3.0 and in Firefox 3.0, the problem will correct itself when the browser is closed and reopened.

On a side note, implementing the fix for bug 350558 in Firefox 2.0 will not allow this session to load correctly since the sessionstore.js file was already corrupted and once it loads, the aTabData.entries.forEach error will start to occur in the error console periodically as Firefox tries to save the session data.  

Fixing 350558 would prevent getting such sessionstore.js files in the first place, but it won't fix existing files.  

I'm going to put a work around in Session Manager to fix the corrupted windows.tabs.entries array, but I figured I'd mention that this could still affect people even if they aren't running Session Manager until they upgrade to Firefox 3.0.

Comment 10

[Simon Bünzli]

Attachment: minimal fix (branch only)

This gracefully handles defect session data we handed through our API to an extension (cf. Michael's patch above) and makes sure that we handle Arrays from the sandbox correctly during the JSON encoding (cf. <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/loader/JSON.jsm&rev=1.2&mark=111-113#109>).

Comment 11

[Simon Bünzli]

Comment on attachment 291891 [details] [diff] [review]
minimal fix (branch only)

Dietrich: See comment #10 above.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 291891 [details] [diff] [review]
minimal fix (branch only)

r=me

Comment 13

[Simon Bünzli]

Comment on attachment 291891 [details] [diff] [review]
minimal fix (branch only)

Branch drivers: This is a low-risk fix which is required to allow extensions to correctly retrieve session data through the SessionStore API (regression introduced in bug 367605 for Firefox 2.0.0.8).

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 291891 [details] [diff] [review]
minimal fix (branch only)

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 15

[Reed Loden [:reed]]

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.5.2.49; previous revision: 1.5.2.48
done

Comment 16

[Samuel Sidler (old account; do not CC)]

Michael, can you confirm this is fixed in Firefox 2.0.0.12 RC 1?

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.12-candidates/rc1/

Comment 17

[Michael Kraft [:morac]]

I tried loading one of the damaged sessions that gets stuck in "loading...".  It still did so, but I could resave it and then load that session and it worked.  I am getting an error when loading the new session, but everything seems to load correctly so I'm not sure what it's problem is.  The error is:

Error: [Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Firefox/components/nsSessionStore.js :: sss_saveState :: line 1753"  data: no]
Source File: file:///C:/Program%20Files/Mozilla%20Firefox/components/nsSessionStore.js
Line: 1753

This is in saveState function line:

    oState.session = { state: ((this._loadState == STATE_RUNNING) ? STATE_RUNNING_STR : STATE_STOPPED_STR) };

Comment 18

[Michael Kraft [:morac]]

Just an update.  After restarting Firefox, I no longer get that error when loading the corrected session file, so it must have been caused by the initial loading of the corrupted file.

Like I mentioned when I forced the attached sessionstore.js file to load it failed to completely load, but saving it as a session seems to correct whatever was wrong with it so I'll say this is as fixed as it's going to get in Firefox 2.0.x

I will mention, I rarely saw this bug myself, but a number of people reported it to me who saw it a lot.  I'll see if I can get some of them to try out 2.0.0.12 RC 1.

Comment 19

[Simon Bünzli]

(In reply to comment #17)
> Error: [Exception... "Illegal value"  nsresult: "0x80070057

This looks like another manifestation of either bug 366509 or bug 410060 which might so far looks like an incompatibility with Firebug.

Comment 20

[Michael Kraft [:morac]]

Since I don't have Firebug installed, I'm guessing it's the former.

Comment 21

[Samuel Sidler (old account; do not CC)]

(In reply to comment #18)
> I will mention, I rarely saw this bug myself, but a number of people reported
> it to me who saw it a lot.  I'll see if I can get some of them to try out
> 2.0.0.12 RC 1.

Thanks Michael. Let me know what they say. :)