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

RESOLVED FIXED in Firefox 53

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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. :-(
(Assignee)

Updated

2 years ago
Component: DOM: Security → General
Product: Core → Firefox
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
status-firefox53: --- → affected
(Assignee)

Comment 2

2 years ago
(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

2 years ago
mozreview-review
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.
Attachment #8830237 - Flags: review?(ckerschb) → review+
(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.
Priority: -- → P1
Whiteboard: [domsecurity-active]

Comment 5

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/03cc3fbf7ae3
use serializeToString instead of non-existant serializePrincipal, r=ckerschb
(Assignee)

Updated

2 years ago
Depends on: 1333727
(Assignee)

Comment 6

2 years ago
(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
(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!
(Assignee)

Comment 8

2 years ago
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
Attachment #8830237 - Flags: approval-mozilla-aurora?

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/03cc3fbf7ae3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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.
Attachment #8830237 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c82f913134cb
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.