Closed Bug 1289785 Opened 8 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ckerschb, Assigned: himshijain.hj)

References

Details

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

Attachments

(1 file, 5 obsolete files)

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
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.
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog3]
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)
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)
Attachment #8801476 - Attachment is patch: false
Attachment #8801477 - Attachment is patch: false
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)
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)
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+
Attachment #8801483 - Flags: review?(fbraun)
Attachment #8801483 - Flags: review?(ckerschb)
Attachment #8801483 - Flags: feedback?(fbraun)
Attachment #8801483 - Flags: feedback?(ckerschb)
Attachment #8801483 - Flags: feedback+
Assignee: nobody → himshijain.hj
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)
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+
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
Okay. So can I and I should I change it now?
Okay. So can I and should I change it now?
(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)
(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)
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)]
Okay Sir I will update it.
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+
(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)
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
https://hg.mozilla.org/mozilla-central/rev/2afcab6fc172
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.