1333726 - Fix use of principal serialization in browser.js to actually work

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[:Gijs (he/him)]

In bug 1331686 we added code that serializes principals. The patch initially used the sessionstore utils which wrap the serialization helper with a method called "serializePrincipal". Unfortunately, the final checkin continues to use that method name but calls it on the serialization helper, where it doesn't exist. I should probably have noticed this in review. :-(

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1333726 - use serializeToString instead of non-existant serializePrincipal,

Review commit: https://reviewboard.mozilla.org/r/107132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107132/

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs from comment #0)
> In bug 1331686 we added code that serializes principals. The patch initially
> used the sessionstore utils which wrap the serialization helper with a
> method called "serializePrincipal". Unfortunately, the final checkin
> continues to use that method name but calls it on the serialization helper,
> where it doesn't exist. I should probably have noticed this in review. :-(

Checking the comments on bug 1331686, my initial review feedback mistakenly referred to:

> and then in here use gSerializationHelper.serializePrincipal instead.

Sorry. :-(

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8830237 [details]
Bug 1333726 - use serializeToString instead of non-existant serializePrincipal,

https://reviewboard.mozilla.org/r/107132/#review108224

r=me, thanks for fixing. This needs uplifting too.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #2)
> > and then in here use gSerializationHelper.serializePrincipal instead.
> 
> Sorry. :-(

Yeah, no worries, I could/should have checked myself. Interestingly is that no test triggered that codepath.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03cc3fbf7ae3
use serializeToString instead of non-existant serializePrincipal, r=ckerschb

Comment 6

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Yeah, no worries, I could/should have checked myself. Interestingly is that
> no test triggered that codepath.

Yes. Filed bug 1333727

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #6)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> > Yeah, no worries, I could/should have checked myself. Interestingly is that
> > no test triggered that codepath.
> 
> Yes. Filed bug 1333727

Awesome - thank you!

Comment 8

[:Gijs (he/him)]

Comment on attachment 8830237 [details]
Bug 1333726 - use serializeToString instead of non-existant serializePrincipal,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1331686
[User impact if declined]: some loads fail
[Is this code covered by automated tests?]: in general, yes. This specific failure, apparently not. We'll look at tests in bug 1333727, but this is such a trivial change that I think it's worth landing on aurora even before the tests are done.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: I don't think so, the change is too trivial to bother (please see patch and other bug comments)
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple change to correct a method name we're calling
[String changes made/needed]: no

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/03cc3fbf7ae3

Comment 10

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

Comment on attachment 8830237 [details]
Bug 1333726 - use serializeToString instead of non-existant serializePrincipal,

Should fix some user issues with page loading, let's uplift for 53.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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