Remove entry.owner_b64 from SessionStore.js and SessionHistory.jsm

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM: Security
P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: ckerschb, Assigned: Himanshi Jain)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [domsecurity-backlog3][do not land before next merge (Nov 8th)])

Attachments

(1 attachment, 5 obsolete attachments)

Within Bug 1286472 we are replacing owner with triggeringPrincipal. To remain backward compatible we still have to support entry.owner_b64 for a few cycles; after that we we can remove those bits within:

browser/components/sessionstore/SessionHistory.jsm
mobile/android/components/SessionStore.js
(Reporter)

Updated

a year ago
Blocks: 1286472
Priority: -- → P3
Whiteboard: [domsecurity-backlog]
This would be nice since these moderately large strings can build up a lot of duplicates adding to noticeable amounts of memory.  See bug 1287906.
We're not removing the strings.  We just changed what property name they're stored under.
(Reporter)

Updated

a year ago
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog3]
(Assignee)

Comment 3

a year ago
Created attachment 8801476 [details]
SessionHistory.jsm(mozilla-central/browser/components/sessionstore/SessionHistory.jsm)

I have removed the following code from the file (i.e. removed entry.owner_b64) in my patch:

// The field aEntry.owner_b64 got renamed to aEntry.triggeringPricipal_b64 in
    // Bug 1286472. To remain backward compatible we still have to support that
    // field for a few cycles before we can remove it within Bug 1289785.
    if (aEntry.owner_b64) {
      aEntry.triggeringPricipal_b64 = aEntry.owner_b64;
      delete aEntry.owner_b64;
    }
Attachment #8801476 - Flags: review?(fbraun)
Attachment #8801476 - Flags: review?(ckerschb)
Attachment #8801476 - Flags: feedback?(fbraun)
Attachment #8801476 - Flags: feedback?(ckerschb)
(Assignee)

Comment 4

a year ago
Created attachment 8801477 [details]
SessionStore.js (mozilla-central/mobile/android/components/SessionStore.js)

I have removed the following code from the file (i.e. removed entry.owner_b64) in my patch:

// The field aEntry.owner_b64 got renamed to aEntry.triggeringPricipal_b64 in
    // Bug 1286472. To remain backward compatible we still have to support that
    // field for a few cycles before we can remove it within Bug 1289785.
    if (aEntry.owner_b64) {
      aEntry.triggeringPricipal_b64 = aEntry.owner_b64;
      delete aEntry.owner_b64;
    }
Attachment #8801477 - Flags: review?(fbraun)
Attachment #8801477 - Flags: review?(ckerschb)
Attachment #8801477 - Flags: feedback?(fbraun)
Attachment #8801477 - Flags: feedback?(ckerschb)
(Assignee)

Updated

a year ago
Attachment #8801476 - Attachment is patch: false
(Assignee)

Updated

a year ago
Attachment #8801477 - Attachment is patch: false
(Assignee)

Comment 5

a year ago
Created attachment 8801482 [details] [diff] [review]
bug_1289785_remove_owner_from_sessionstore.patch
Attachment #8801477 - Attachment is obsolete: true
Attachment #8801477 - Flags: review?(fbraun)
Attachment #8801477 - Flags: review?(ckerschb)
Attachment #8801477 - Flags: feedback?(fbraun)
Attachment #8801477 - Flags: feedback?(ckerschb)
Attachment #8801482 - Flags: review?(fbraun)
Attachment #8801482 - Flags: review?(ckerschb)
Attachment #8801482 - Flags: feedback?(fbraun)
Attachment #8801482 - Flags: feedback?(ckerschb)
(Assignee)

Comment 6

a year ago
Created attachment 8801483 [details] [diff] [review]
bug_1289785_remove_owner_from_sessionhistory.patch
Attachment #8801476 - Attachment is obsolete: true
Attachment #8801476 - Flags: review?(fbraun)
Attachment #8801476 - Flags: review?(ckerschb)
Attachment #8801476 - Flags: feedback?(fbraun)
Attachment #8801476 - Flags: feedback?(ckerschb)
Attachment #8801483 - Flags: review?(fbraun)
Attachment #8801483 - Flags: review?(ckerschb)
Attachment #8801483 - Flags: feedback?(fbraun)
Attachment #8801483 - Flags: feedback?(ckerschb)
(Reporter)

Comment 7

a year ago
Comment on attachment 8801482 [details] [diff] [review]
bug_1289785_remove_owner_from_sessionstore.patch

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

Looks good to me. JFYI, no need, to flag the same person using r? as well as f? ;-)

Please squash the two changesets into one and flag me again. If everything is fine I can forward the r? request to an actual peer owner.

Thanks!
Attachment #8801482 - Flags: review?(fbraun)
Attachment #8801482 - Flags: review?(ckerschb)
Attachment #8801482 - Flags: feedback?(fbraun)
Attachment #8801482 - Flags: feedback?(ckerschb)
Attachment #8801482 - Flags: feedback+
(Reporter)

Updated

a year ago
Attachment #8801483 - Flags: review?(fbraun)
Attachment #8801483 - Flags: review?(ckerschb)
Attachment #8801483 - Flags: feedback?(fbraun)
Attachment #8801483 - Flags: feedback?(ckerschb)
Attachment #8801483 - Flags: feedback+
(Reporter)

Updated

a year ago
Assignee: nobody → himshijain.hj
(Assignee)

Comment 8

a year ago
Created attachment 8801652 [details] [diff] [review]
bug_1289785_remove_owner_from_sessionstore_and_sessionhistory.patch

Okay I will keep in mind that the same person need not be tagged for both. Does it look fine now?
Attachment #8801482 - Attachment is obsolete: true
Attachment #8801483 - Attachment is obsolete: true
Attachment #8801652 - Flags: review?(ckerschb)
(Reporter)

Comment 9

a year ago
Comment on attachment 8801652 [details] [diff] [review]
bug_1289785_remove_owner_from_sessionstore_and_sessionhistory.patch

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

Mike, Bug 1286472 landed within FF50, is it too early to remove |entry.owner_b64| or can we go ahead and remove it?
Attachment #8801652 - Flags: review?(mdeboer)
Attachment #8801652 - Flags: review?(ckerschb)
Attachment #8801652 - Flags: review+
(Reporter)

Comment 10

a year ago
Himanshi, either way before landing you need to update the commit message of the bug. It needs to contain the bug number as well as reviewers, something like:

Bug 1289785 - Remove entry.owner_b64 from SessionStore.js and SessionHistory.jsm. r=ckerschb,mdeboer
Status: NEW → ASSIGNED
(Assignee)

Comment 11

a year ago
Okay. So can I and I should I change it now?
(Assignee)

Comment 12

a year ago
Okay. So can I and should I change it now?
(Reporter)

Comment 13

a year ago
(In reply to Himanshi Jain from comment #12)
> Okay. So can I and should I change it now?

Let's wait for Mike's reply first. Maybe he suggests to modify the patch. In that case you wouldn't have to upload a new patch twice :-)
Hi Cristoph, r=me if 'owner_b64' wasn't available yet in Fx 45 ESR. Otherwise we'll need to keep it in 52 as well, which will become the next ESR.
Flags: needinfo?(ckerschb)
(Reporter)

Comment 15

a year ago
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> Hi Cristoph, r=me if 'owner_b64' wasn't available yet in Fx 45 ESR.
> Otherwise we'll need to keep it in 52 as well, which will become the next
> ESR.

Thanks Mike - a quick DXR search [1] confirms we have to wait till FF52 before we can land this bug.

[1] https://dxr.mozilla.org/mozilla-esr45/search?q=entry.owner_b64
Flags: needinfo?(ckerschb)
(Reporter)

Comment 16

a year ago
Himanshi, please just update the commit message and then we can let it sit here till November 8th - this is when the next merge happens and then we can land this patch - thanks for fixing!
Flags: needinfo?(himshijain.hj)
Whiteboard: [domsecurity-backlog3] → [domsecurity-backlog3][do not land before next merge (Nov 8th))
Whiteboard: [domsecurity-backlog3][do not land before next merge (Nov 8th)) → [domsecurity-backlog3][do not land before next merge (Nov 8th)]
(Assignee)

Comment 17

a year ago
Okay Sir I will update it.
(Assignee)

Comment 18

a year ago
Created attachment 8802953 [details] [diff] [review]
bug_1289785_remove_owner_from_sessionstore_and_sessionhistory.patch

Does it seem fine now? Are there any other changes that I need to do?
Attachment #8801652 - Attachment is obsolete: true
Attachment #8801652 - Flags: review?(mdeboer)
Comment on attachment 8802953 [details] [diff] [review]
bug_1289785_remove_owner_from_sessionstore_and_sessionhistory.patch

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

LGTM! Thanks, Himanshi!
Attachment #8802953 - Flags: review+
(Reporter)

Comment 20

a year ago
(In reply to Christoph Kerschbaumer [:ckerschb (limited availability)] from comment #16)
> Himanshi, please just update the commit message and then we can let it sit
> here till November 8th - this is when the next merge happens and then we can
> land this patch - thanks for fixing!

Oh, you forgot to clear the needinfo, but the commit message seems fine now - thanks!
Flags: needinfo?(himshijain.hj)
Keywords: checkin-needed

Comment 21

8 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2afcab6fc172
Remove entry.owner_b64 from SessionStore.js and SessionHistory.jsm. r=ckerschb, r=mdeboer
Keywords: checkin-needed

Comment 22

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2afcab6fc172
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.