1016759 - session restore not fully working

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Sean Smith]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 SeaMonkey/2.26 (Beta/Release)
Build ID: 20140428215944

Steps to reproduce:

Set "Restore previous session" for Browser startup.  Closed browser.  Restarted browser.


Actual results:

Browsing session is remembered and restored.  However, the following are not restored:
1 - Page scroll position
2 - Page Style
3 - Form / text box entries that have not been submitted
4 - Entry in about:config search filter

Example pages missing page style upon restore:
any bugzilla.mozilla.org bug page
http://www.seamonkey-project.org/

Most pages restore page style, but for me, NO pages restore scroll position or form entries.

Discussion page at mozillaZine:
http://forums.mozillazine.org/viewtopic.php?f=4&t=2836085


Expected results:

Open tabs and windows should be restored, inclusive of scroll position, page style, not submitted form entries, etc.

Comment 1

[Sean Smith]

A couple of Firefox bugs that probably missed the 2.26 release that would fix this:
https://bugzilla.mozilla.org/show_bug.cgi?id=921942
https://bugzilla.mozilla.org/show_bug.cgi?id=952401

Comment 2

[Jiri TRAVNICEK [:JITR]]

Can confirm on 32bit Windows 7 as well.

Don't see the defect at `bugzilla.mozilla.org' though (which is probably because there seems to be only a default stylesheet), but can reproduce at `bugzilla-dev.allizom.org'. The defect can also be observed after a window is closed and then re-opened.

Additionally, it seems the session restore itself works okay, the problem is only with the save -- the saved `sessionstore.json' is missing `pageStyle' and `scroll' properties. If these were present (e. g. after upgrade from SM 2.25), they got restored as expected (and would get lost upon next save).

Comment 3

[Jiri TRAVNICEK [:JITR]]

After a couple of quick & dirty tests (by modifying a file inside `omni.ja') I have narrowed the cause to the of changeset `698afc56b076' (landed as part of bug 869900). With only part of this changeset applied problem still didn't occur, but it reappeared right after applying this hunk:

--- a/suite/common/src/nsSessionStore.js
+++ b/suite/common/src/nsSessionStore.js
@@ -1741,18 +1738,17 @@ SessionStoreService.prototype = {
    * @param aWindow
    *        Window reference
    */
   _updateTextAndScrollData: function sss_updateTextAndScrollData(aWindow) {
     var browsers = aWindow.getBrowser().browsers;
     for (var i = 0; i < browsers.length; i++) {
       try {
         var tabData = this._windows[aWindow.__SSi].tabs[i];
-        if (browsers[i].__SS_data &&
-            browsers[i].__SS_tabStillLoading)
+        if (browsers[i].__SS_data)
           continue; // ignore incompletely initialized tabs
         this._updateTextAndScrollDataForTab(aWindow, browsers[i], tabData);
       }
       catch (ex) { debug(ex); } // get as much data as possible, ignore failures (might succeed the next time)
     }
   },
 
   /**

Comment 4

[Jiri TRAVNICEK [:JITR]]

Just to prevent any misunderstanding: I'm currently not actively working on this. I have contributed what information I was able to gather about this bug with the hope of helping someone familiar enough with the code involved in finding a proper fix. Getting familiar with the code seems to be necessary and I don't think I could do it with a reasonable effort and in a reasonable time. :-(

If this stays unfixed much longer, I might try to take on, but I'd rather not promise anything yet.

Comment 5

[Masato]

I too experienced this too:

For messing up the style : https://forums.wildstar-online.com/forums/index.php?/forum/153-deutsch/

If I change to another tab, closing SM, reopening it and if I see the tab is loading I switch back to the tab I see this:
A correctly displayed website is loading and at the moment of finishing: the whole layout, color etc messes up at once.
(if this is the wrong bug I post this in please give me a message)

Comment 6

[Justin Wood (:Callek)]

per IRC affects multiple platforms on at least SeaMonkey 2.29

Comment 7

[lemon_juice]

Jiri TRAVNICEK in Comment 3 posted useful information on which change in the code triggered the bug. Working from there I think I have found the cause of the bug along with the solution.

Changes in attachment 8338012 [details] [diff] [review] got rid of browser.__SS_tabStillLoading property, which was used in some conditional statements and was essential in determining whether "TextAndScrollDataForTab" were actually saved or not - therefore simply removing it from the conditionals wasn't enough. I've found out that this property can be substituted with checking the value of browser.__SS_restoreState.

I'm presenting a simple patch that adds this check to two conditional statements that used to check for browser.__SS_tabStillLoading property in the past. The first one and the crucial one is in line 1735 - the simple "if (browsers[i].__SS_data)" caused the _updateTextAndScrollDataForTab() function to be skipped probably under any conditions. The second one - analogous - in line 1420 where there is different code for saving session data depending on whether the tab is loaded or not. I haven't really noticed any difference in practical working of session restore after updating the conditional in line 1420 - but I added it for consistency sake, since there might be some reasons unknown to me why the code is different for loaded and non-loaded tabs so I think it's safer to have the proper conditional check in that spot.

I have applied the patch in omni.ja to both SM 2.32 Beta 2 and SM 2.33 aurora from 2014-12-31 and session restore correctly restores both the scroll position and styles on pages with named style sheets so in my experience this fixes the problems reported here. The analogous changes can be applied also to 2.31 and therefore probably to all version down to 2.26. I have tested it with "Only restore the tabs when I need them" as well as the standard settings and it seems to work fine.

I'll try to post the patch now, please forgive me if I don't do it properly, because I've never posted a patch before. I don't know how to build SeaMonkey and do all the proper tests so I'm presenting this patch to make it easier for someone to pinpoint the problem and finally apply it to the source.

Comment 8

[lemon_juice]

Attachment: Fix restoring scroll position and user styles

This patch can be applied both to SM2.32b2 and 2.33a build 20141231

Comment 9

[Philip Chee]

Comment on attachment 8542960 [details] [diff] [review]
Fix restoring scroll position and user styles

> I'll try to post the patch now, please forgive me if I don't do it properly,
> because I've never posted a patch before. I don't know how to build
> SeaMonkey and do all the proper tests so I'm presenting this patch to make
> it easier for someone to pinpoint the problem and finally apply it to the
> source.
Your patch looks fine! Setting review request to Neil@parkway

Comment 10

[[e-mail]]

Comment on attachment 8542960 [details] [diff] [review]
Fix restoring scroll position and user styles

Sorry I don't really understand this change (or the linked one, but IanN reviewed it, so he's getting this one too).

Comment 11

[Ian Neal]

Comment on attachment 8542960 [details] [diff] [review]
Fix restoring scroll position and user styles

r=me

Comment 12

[lemon_juice]

Great, glad to see some activity in this bug!

(In reply to [e-mail] from comment #10)
> Comment on attachment 8542960 [details] [diff] [review]
> Fix restoring scroll position and user styles
> 
> Sorry I don't really understand this change (or the linked one, but IanN
> reviewed it, so he's getting this one too).
You mean you don't understand what the proposed changes do? Actually, they don't add anything new but bring back the two (important) conditional checks involving browser.__SS_tabStillLoading that were removed in Attachment #8338012 [details] [diff]. Because browser.__SS_tabStillLoading doesn't exist any more after that change it doesn't mean the checks are unnecessary and could simply be discarded as they were. I'm simply bringing them back using a different method that checks for browser.__SS_restoreState value.

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Seems this was already pushed to trunk (comm-central changeset 964092fa6430). Judging from comment 7 I take it that Aurora and Beta need the fix as well?

Comment 14

[lemon_juice]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #13)
> Seems this was already pushed to trunk (comm-central changeset
> 964092fa6430). Judging from comment 7 I take it that Aurora and Beta need
> the fix as well?

Yes!

Comment 15

[Phoenix]

Comment on attachment 8542960 [details] [diff] [review]
Fix restoring scroll position and user styles

[Approval Request Comment]
Regression caused by (bug #): Bug 869900
User impact if declined: Scroll positions will continue to not be restored for next two release cycles
Testing completed (on m-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none

Comment 16

[Philip Chee]

Comment on attachment 8542960 [details] [diff] [review]
Fix restoring scroll position and user styles

a=me for comm-aurora and comm-beta

Comment 17

[Philip Chee]

Pushed to branches.
http://hg.mozilla.org/releases/comm-aurora/rev/041dfe66d51f
http://hg.mozilla.org/releases/comm-beta/rev/3f3212e3dfe5