1338009 - Utils#deserializePrincipal should return NullPrincipal if deserialization fails

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mike de Boer [:mikedeboer] from comment #24) from Bug 1334875 [1]

> This would mean that
> 1) we'd add a try...catch in Utils#deserializePrincipal, along with a
> debug() method as in SessionHistory.jsm - which may also log the
> principal_b64 data it failed to deserialize
> 2) return `null` upon failure and
> 3) clean out the try..catches from SessionHistory.jsm

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1334875#c24

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1338009_deserialiceprincipal_do_return_nullprincipal.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Locally verified that all tests within browser/components/sessionstore/test/ pass. Let me know if we need a try run for that as well.

Comment 3

[:Gijs (out for now; he/him)]

Comment on attachment 8835236 [details] [diff] [review]
bug_1338009_deserialiceprincipal_do_return_nullprincipal.patch

Review of attachment 8835236 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/sessionstore/Utils.jsm
@@ +141,5 @@
> +      principal.QueryInterface(Ci.nsIPrincipal);
> +      return principal;
> +    } catch (e) {
> +      debug("principal_b64 (" + principal_b64 + ")" + e);
> +      return Services.scriptSecurityManager.createNullPrincipal({});

Right now we return |null| in the similar case of not having a principal at all, wouldn't that make more sense? It's very much not the same as an explicit null principal, especially now that we fall back to using system principal if no triggering principal was stored...

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #3)
> Right now we return |null| in the similar case of not having a principal at
> all, wouldn't that make more sense? It's very much not the same as an
> explicit null principal, especially now that we fall back to using system
> principal if no triggering principal was stored...

I was thinking about that as well, and I am totally fine with updating to |return null;| in that case. I was thinking that using a NullPrincipal would result in a nice Console message if the load fails.

JFYI, ideally we would like to return an error if the history entry does not have a triggeringPrincipal within LoadHistoryEntry, we only temporarily fall back to using the systemPrincipal. But I suppose that was the reason you brought it up, right? In other words, returning null if deserialization fails definitely causes the load to succeed because of the fallback to systemprincipal within docshell.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> JFYI, ideally we would like to return an error if the history entry does not
> have a triggeringPrincipal within LoadHistoryEntry, we only temporarily fall
> back to using the systemPrincipal. But I suppose that was the reason you
> brought it up, right? In other words, returning null if deserialization
> fails definitely causes the load to succeed because of the fallback to
> systemprincipal within docshell.

Yes.

Really, it feels to me like, while this is a reasonable band-aid, we should version the sessionstore file and just 'fill in' system principal for referrer-less history entries when we encounter an old sessionstore file that misses these. Then we can require them being present afterwards.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8835236 [details] [diff] [review]
bug_1338009_deserialiceprincipal_do_return_nullprincipal.patch

Review of attachment 8835236 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed. Thanks Christoph!

::: toolkit/modules/sessionstore/Utils.jsm
@@ +140,5 @@
> +      let principal = serializationHelper.deserializeObject(principal_b64);
> +      principal.QueryInterface(Ci.nsIPrincipal);
> +      return principal;
> +    } catch (e) {
> +      debug("principal_b64 (" + principal_b64 + ")" + e);

nit: debug(`Failed to deserialize principal_b64 '${principal_b64}' ${e}`);

@@ +141,5 @@
> +      principal.QueryInterface(Ci.nsIPrincipal);
> +      return principal;
> +    } catch (e) {
> +      debug("principal_b64 (" + principal_b64 + ")" + e);
> +      return Services.scriptSecurityManager.createNullPrincipal({});

I think I agree with Gijs that returning `null` explicitly is clearer and mirrors the current situation.
Can you also put that outside of the `catch` clause?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> I think I agree with Gijs that returning `null` explicitly is clearer and
> mirrors the current situation.

Yeah, I think we all agree on that now.

> Can you also put that outside of the `catch` clause?

Certainly. Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/739f1c4bee0d
Utils#deserializePrincipal should return NullPrincipal if deserialization fails. r=mikedeboer

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/739f1c4bee0d

Comment 10

[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

This new SeaMonkey nightly does not suffer from the "pages blank on session restore" bug 1334780 / bug 1334875, which was present on the previous (Feb. 9) nightly. AFAIK no patch was attached to either of those bugs. Feel free to set VERIFIED here if it means that this fix was enough to cure the problem.

Comment 11

[Tony Mechelynck [:tonymec]]

Hm, Somehow mozilla-central changeset 00d16f03506b was a merge changeset pushed about 1½ days _before_ mozilla-central changeset 739f1c4bee0d mentioned in comment #9. Now I don't know what to think.

Comment 12

[Tony Mechelynck [:tonymec]]

(In reply to Tony Mechelynck [:tonymec]. (NEEDINFO me if you want my attention) from comment #11)
> Hm, Somehow mozilla-central changeset 00d16f03506b was a merge changeset
> pushed about 1½ days _before_ mozilla-central changeset 739f1c4bee0d
> mentioned in comment #9. Now I don't know what to think.

oops, only ½ days (12 hours ±1)

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Would it make sense to uplift this to aurora, to fix bug 1334875 in 53?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8835236 [details] [diff] [review]
bug_1338009_deserialiceprincipal_do_return_nullprincipal.patch

Approval Request Comment
The changeset within this Bug as well as the changes within Bug 1337622 (which already got uplifted) will most certainly fix the problem described within Bug 1334875. Even though we are not 100% certain, we are still confident. On top the changeset within this bug is very safe to be uplifted.

[Feature/Bug causing the regression]:
Bug 1307736 - Assert history loads pass a valid triggeringPrincipal for docshell loads

[User impact if declined]:
Users might lose data due do the fact that session restoration might not work correctly.

[Is this code covered by automated tests?]:
No. Kamil is on it to figure out STRs.

[Has the fix been verified in Nightly?]:
No, because we are still missing reliable STRs. Kamil is on it.

[Needs manual test from QE? If yes, steps to reproduce]:
Once we know the STRs, then Kamil will verify.

[List of other uplifts needed for the feature/fix]:
Bug 1337622 - Temporarily fall back to SystemPrincipal if History entry does not have a valid triggeringPrincipal

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Explicitly returning null instead of throwing an exception.

[String changes made/needed]:
no

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8835236 [details] [diff] [review]
bug_1338009_deserialiceprincipal_do_return_nullprincipal.patch

Fix for a session restore issue, let's get it into aurora and verify it there.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b47862d310fa

Comment 17

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Possible fix for bug 1345456

FYI, I did a Try push of this backported to Fx52 and things appear to be OK from a CI standpoint. Not sure what kind of manual testing would be prudent, though.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4095426dd201d44a4db9b4cda918977d25de70c&group_state=expanded

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Users confirm that backporting this to 52 fixes the problems in bug 1345456. Christoph, what are your thoughts about backporting this is a dot release ride-along (and ESR52)?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Attachment: patch rebased for m-r/esr52

Here's the rebased patch I ran through Try. There was only one trivial conflict.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Users confirm that backporting this to 52 fixes the problems in bug 1345456.
> Christoph, what are your thoughts about backporting this is a dot release
> ride-along (and ESR52)?

Yes, we should do that. Is there anything you need from my side to make this happen? Thanks for rebasing and applying the patch on top of ESR52.

Comment 21

[Julien Cristau [:jcristau]]

Tracking as 52 dot release candidate.  Christoph, maybe you can fill in the uplift request template for release and esr52 on attachment 8848914 [details] [diff] [review]?

Comment 22

[:Gijs (out for now; he/him)]

(In reply to Julien Cristau [:jcristau] from comment #21)
> Tracking as 52 dot release candidate.  Christoph, maybe you can fill in the
> uplift request template for release and esr52 on attachment 8848914 [details] [diff] [review]
> [details] [diff] [review]?

+ni

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8848914 [details] [diff] [review]
patch rebased for m-r/esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Users are facing session restoration problems. One particular instance of that problem is 'Bug 1345456 - Favicon fails to display for majority of tabs upon restart' where the triggeringPrincipal can not be deserialized.

Fix Landed on Version:
Fix is landed and uplifted all the way to FF53.

Risk to taking this patch (and alternatives if risky): 
No, not risky. In fact deserialization of a principal explicitly returns null instead of throwing an exception.

String or UUID changes made by this patch: 
No

Approval Request Comment
See previous and also comment 14.
Please note that even though comment 14 mentions some other bug as the root cause of this problem. Actually Bug 1297338 introduced the problem we are trying to solve on FF52. So no need to uplift the other patches mentioned in comment 14 as well, only this rebased one.

[Feature/Bug causing the regression]:
Bug 1297338 - Introduce the concept of principalToInherit to loadinfo

[User impact if declined]:
See previous.

[Is this code covered by automated tests?]:
Ryanvm provided a rebased patch and users [1,2] verified it fixes the problem.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1345456#c56
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1345456#c57

[Has the fix been verified in Nightly?]:
No - not that I know of.

[Needs manual test from QE? If yes, steps to reproduce]:
STRs are provided here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1345456#c0

[List of other uplifts needed for the feature/fix]:
No other patches needed.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Updated so deserialization of a principal explicitly returns null instead of throwing an exception.

[String changes made/needed]:
No

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8848914 [details] [diff] [review]
patch rebased for m-r/esr52

thanks, let's get this in 52, seems to affect quite a few people judging from bug 1345456

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/b537e54224af

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d85776323602 (default - 52.1.0)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/0f308ec35f67 (FIREFOX_ESR_52_0_X_RELBRANCH - 52.0.2)

Comment 28

[Andrei Vaida [:avaida]]

Setting qe-verify- based on the fact that this fix has automated coverage (see Comment 23).

Comment 29

[reikred]

I can report that although version 52.0.2 initially fixed the problem I know have seen again that large amounts of favicons are missing  when I had to restart my firefox about 2 days ago.

Whatever the fix was, it turned out not to last and something tripped things up to such an extent that I am again missing 100s of favicons.

Fedora release 23 (Twenty Three)
Linux version 4.8.13-100.fc23.x86_64 ([e-mail]) (gcc version 5.3.1 20160406 (Red Hat 5.3.1-6) (GCC) ) #1 SMP Fri Dec 9 14:51:40 UTC 2016

Comment 30

[reikred]

I have now also tested with FF 53.0. Still missing 100s of favicons from my session. THE SUBJECT BUG HAS NOT BEEN FIXED, OR THERE ARE ADDITIONAL BUGS THAT CAUSE THE SAME PROBLEM.