1334780 - Session restore only shows blank page instead of contents for page with URL correctly shown in location bar

Status: VERIFIED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Frank-Rainer Grahl (:frg)]

Session restore got broken between 01/24 and 01/28. After restoring the browser the location bar will show the url but only a blank page will be shown. No errors in either browser or error console. Reload will not help. You need to select the location bar and press Enter to reload the page.

I suspect bug 1307736 but have not yet investigated.

Comment 1

[Frank-Rainer Grahl (:frg)]

Imho at least Bug 1286472 and Bug 1307736 need to be ported.

Comment 2

[Frank-Rainer Grahl (:frg)]

Attachment: 1334780-SessionRestore.patch

I hope I got it all right.

Comment 3

[Rainer Bielefeld]

NOT reproducible with Server-Installation of  official en-US SeaMonkey 2.51a1 (NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0 Build 20170125003834  (Default Classic Theme) on German WIN7 64bit:

Neither after CRASH (provoked due to Bug 1224326) nor after normal SM relaunch I see the described effect, session restore works fine for me.

Comment 4

[Frank-Rainer Grahl (:frg)]

You need a build after Bug 1307736 landed to see this so anything before 01/28 won't show it.

This commit needs to be in it:

https://hg.mozilla.org/mozilla-central/rev/f6379ae84275

Comment 5

[Rainer Bielefeld]

NOT (no longer?) reproducible with Server-Installation of  unofficial (by FRG)  en-US SeaMonkey 2.51a1 (NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0 Build 20170129150105  (Default Classic Theme) on German WIN7 64bit.

Due to FRG by private email fix ist integrated in that build, and indeed, neither after CRASH (provoked due to Bug 1224326) nor after normal SM relaunch I see the described effect, session restore works fine for me.

But:

I still am able to provoke the blank pages with a (rather unusual) proceeding:

0. Launch SM  Build 20170125003834 (from comment 3) → make sure that preference
   is "Restore last Session when launch".  → Quit SM → launch 
   Build 20170129150105 → make sure that preference is 
   "Restore last Session when launch" → quit SM 
1. Launch SM  Build 20170125003834 (from comment 3) → in Browser open several 
   TABs with unofficial Seamonkey Blogs, several other wordpress blogs and
   several other pages → wait until all pages have been loaded completely
2. Quit SM
3. Launch Build 20170129150105
   » In Browser TAB headings from Step 2 appear
   Bug: All TABs empty, only grey area instead of web page contents shown.

a) Of course that is not a common problem for many users, but the result might
   show that 
a1) the fix still does not work for all circumstances
a2) or that there is a different problem affecting "save TABs status when quit"

Comment 6

[Rainer Bielefeld]

a2): FRG told by PM that my observations might be related to problems with 
     sessionrestore.json created with older build.

Comment 7

[Frank-Rainer Grahl (:frg)]

> a) Of course that is not a common problem for many users, but the result might
> show that 

The triggeringPrincipal is missing from older sessionstore.json files. I suspect this will happen with any first 2.51 run with an older sessionstore.json. Probably would be best to backport Bug 1286472 to 2.49 and up after this one here is reviewed. Could also go into 2.48.

> } else if (aEntry.triggeringPrincipal_b64) {

Putting another else here if neither aEntry.triggeringPrincipal_base64 nor aEntry.principalToInherit_base64 could maybe also be done. Just set the triggeringPrincipal to a deserialized Utils.SERIALIZED_SYSTEMPRINCIPAL would probably work but make the whole concept as far as I understand it rather pointless.

Comment 8

[Tony Mechelynck [:tonymec]]

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0 SeaMonkey/2.51a1" 
ID:20170203003001 en-US 
c-c:f3b5625cd5a1eae37df4563cc40c766d8cdba239 
m-c:2aede0a97bc685e163196cc451b947a04ae6a598

I see the problem with this build.

Other than hitting Enter in the Location Bar (comment #0) another usable workaround consists of clicking the Go button (after, if necessary, "customizing" it from the button reserve onto a toolbar, e.g. just right of the Location Bar).

Comment 9

[Rainer Bielefeld]

OS due to comment

Comment 10

[Tony Mechelynck [:tonymec]]

"see also" bug 1334875 has been moved from Firefox to Core but according to bug 1334875 comment #17 the present issue is not necessarily a dupe.

Comment 11

[Tony Mechelynck [:tonymec]]

"OS:All → Unspecified"

Huh? I didn't mean to make this change; and I'm setting OS to All to because the Firefox bug happens also on Windows.

Comment 12

[Frank-Rainer Grahl (:frg)]

This has been uplifted. 2.50 is also now affected.

Comment 13

[Tony Mechelynck [:tonymec]]

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0 SeaMonkey/2.51a1" 
ID:20170213003001 en-US 
c-c:da2e8137b3dfe320d00c5b79f5f3c07e82225efe 
m-c:00d16f03506b7f9f754b01a0a458c05445ac6dba

There was a new nightly today (this one) and it doesn't suffer from the bug. I suppose that the bug has been fixed in Core bug 1334875 and I'm setting this as dupe on this assumption.

@frg: if you don't agree, feel free to REOPEN, or if you do agree, to set VERIFIED once it is known that besides my L64 2.51a1 no other platform still suffers from the bug.

Comment 14

[Frank-Rainer Grahl (:frg)]

This is not resolved. Even if you don't see this specific problem any longer the changes to sessionstore need to go into the tree.

Comment 15

[Tony Mechelynck [:tonymec]]

(In reply to Frank-Rainer Grahl from comment #14)
> This is not resolved. Even if you don't see this specific problem any longer
> the changes to sessionstore need to go into the tree.

OK. I don't know what happened then: the only patch relevant to this problem which I can see pushed, happened about 12 hours _after_ mozilla-central changeset 00d16f03506b from which this nightly was built. :-?

Comment 16

[Frank-Rainer Grahl (:frg)]

The try catch around the deserialize can now removed in 2.51 only after bug 1338009. I will do a small addenum patch once this one here is reviewed. Does not impact functionality.

Comment 17

[Ian Neal]

Comment on attachment 8831523 [details] [diff] [review]
1334780-SessionRestore.patch

># HG changeset patch
># User Frank-Rainer Grahl <[e-mail]>
># Parent  c0e5b93e1369ec16a2c0747df816fbcae5084fff
>Bug 1334780 - Support triggeringPrincipal and principalToInherit in SeaMonkey sessionstore.
>Port Bug 1286472 [Replace nsiSupports owner with nsIPrincipal triggeringPrincipal within nsIDocShell]
>Port Bug 1307736 [Assert history loads pass a valid triggeringPrincipal for docshell loads]
and Bug 1297338

>+++ b/suite/common/src/nsSessionStore.js
>-    var entry = { url: aEntry.URI.spec };
>+    var entry = {url: aEntry.URI.spec,
Why remove the space between { and url?

>+    // Collect triggeringPrincipal data for the current history entry.
>+    // Please note that before Bug 1297338 there was no concept of a
>+    // principalToInherit. To remain backward/forward compatible we
>+    // serialize the principalToInherit as triggeringPrincipal_b64.
>+    // Once principalToInherit is well established (within Gecko 55)
>+    // we can update this code, remove triggeringPrincipal_b64 and
>+    // just keep triggeringPrincipal_base64 as well as
>+    // principalToInherit_base64.
>+    if (aEntry.principalToInherit) {
>       try {
>+        let principalToInherit = Utils.serializePrincipal(aEntry.principalToInherit);
>+        if (principalToInherit) {
>+          entry.triggeringPrincipal_b64 = principalToInherit;
>+          entry.principalToInherit_base64 = principalToInherit;
>+        }
>+      } catch (e) {
>+        debug(e);
We debug others dump?
>       }
>+    }
>+
>+    if (aEntry.triggeringPrincipal) {
>+      try {
>+        let triggeringPrincipal = Utils.serializePrincipal(aEntry.triggeringPrincipal);
>+        if (triggeringPrincipal) {
>+          entry.triggeringPrincipal_base64 = triggeringPrincipal;
>+        }
>+      } catch (e) {
>+        debug(e);
We debug others dump?
>+      }

>+    // The field entry.owner_b64 got renamed to entry.triggeringPricipal_b64 in
>+    // Bug 1286472 and Bug 1334780 for SeaMonkey.
>+    // To remain backward compatible we still have to support that field for a
>+    // few cycles before we can remove it.
>     if (aEntry.owner_b64) {
>+      aEntry.triggeringPricipal_b64 = aEntry.owner_b64;
>+      delete aEntry.owner_b64;
>+    }
Bug 1289785 on c-c, but not on c-b and c-a?

>+    if (aEntry.triggeringPrincipal_base64 || aEntry.principalToInherit_base64) {
>+      if (aEntry.triggeringPrincipal_base64) {
>+        try {
>+          shEntry.triggeringPrincipal =
>+            Utils.deserializePrincipal(aEntry.triggeringPrincipal_base64);
>+        } catch (e) {
>+          debug(e);
>+        }
>+      }
>+      if (aEntry.principalToInherit_base64) {
>+        try {
>+          shEntry.principalToInherit =
>+            Utils.deserializePrincipal(aEntry.principalToInherit_base64);
>+        } catch (e) {
>+          debug(e);
>+        }
>+      }
>+    } else if (aEntry.triggeringPrincipal_b64) {
>+      try {
>+        shEntry.triggeringPrincipal = Utils.deserializePrincipal(aEntry.triggeringPrincipal_b64);
>+        shEntry.principalToInherit = shEntry.triggeringPrincipal;
>+      }
>+      catch (e) {
>+        debug(e);
>+      }
>     }
Again with the debug instead of dump?
r/a=me

Comment 18

[Frank-Rainer Grahl (:frg)]

> and Bug 1297338
comment fixed.

> Why remove the space between { and url?
fixed in push and also a trailing space in a new line.

> We debug others dump?
Just does 'Services.console.logStringMessage("SessionStore: " + aMsg)' which I think is ok and used here in most places too. Dump is also used in a few places but I think not appropriate in the patch.

https://hg.mozilla.org/comm-central/rev/b71b7e1bddce4bed681d887aad080bcc216d5cac

Comment 19

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8831523 [details] [diff] [review]
1334780-SessionRestore.patch

[Approval Request Comment]
Regression caused by (bug #): 1307736
User impact if declined: so far none visible because part of bug 1307736 has been backed out.
Testing completed (on m-c, etc.): c-a and c-c
Risk to taking this patch (and alternatives if risky): none. It should go in to store and set the principals.
String changes made by this patch: -

Should I do a beta and probably release only patch for Bug 1286472 only? 
owner_b64 and friends is still used there.

Comment 20

[Ian Neal]

Comment on attachment 8831523 [details] [diff] [review]
1334780-SessionRestore.patch

a=me for c-a
Yes, do the relevant bits for c-b and release

Comment 21

[Frank-Rainer Grahl (:frg)]

https://treeherder.mozilla.org/#/jobs?repo=comm-aurora&revision=04a040abc3fd28cdaf3cbb1f25179353b541df83

Patch for beta and release for Bug 1286472 coming up later.

Comment 22

[Rainer Bielefeld]

Works fine with unzipped installer of  official  en-US SeaMonkey 2.52a1 (NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 Build 20170311002651  (Default Classic Theme) on German WIN7 64bit