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

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

We're not removing the strings.  We just changed what property name they're stored under.

Comment 3

[Himanshi Jain]

Attachment: 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;
    }

Comment 4

[Himanshi Jain]

Attachment: 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;
    }

Comment 5

[Himanshi Jain]

Attachment: bug_1289785_remove_owner_from_sessionstore.patch

Comment 6

[Himanshi Jain]

Attachment: bug_1289785_remove_owner_from_sessionhistory.patch

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 8

[Himanshi Jain]

Attachment: 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?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 11

[Himanshi Jain]

Okay. So can I and I should I change it now?

Comment 12

[Himanshi Jain]

Okay. So can I and should I change it now?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(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 :-)

Comment 14

[Mike de Boer [:mikedeboer]]

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.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(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

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 17

[Himanshi Jain]

Okay Sir I will update it.

Comment 18

[Himanshi Jain]

Attachment: bug_1289785_remove_owner_from_sessionstore_and_sessionhistory.patch

Does it seem fine now? Are there any other changes that I need to do?

Comment 19

[Mike de Boer [:mikedeboer]]

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!

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(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!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2afcab6fc172
Remove entry.owner_b64 from SessionStore.js and SessionHistory.jsm. r=ckerschb, r=mdeboer

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2afcab6fc172