1410067 - Session cookies are using an unsafe expiry max value

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Based on the discussion on bug 1408962 comment 8:

(In reply to Tim Taubert [:ttaubert] from comment #8)
> (In reply to Henrik Skupin (:whimboo) from comment #4)
> > Interesting fact Peter! Here is the exact description:
> > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> > Global_Objects/Number/MAX_SAFE_INTEGER
> > 
> > Which makes me wonder why we make use of the following line of code:
> > https://dxr.mozilla.org/mozilla-central/source/browser/components/
> > sessionstore/SessionCookies.jsm#20-21
> > 
> > Tim, you added that code via bug 903398. Can you please have a look if we
> > currently have faulty behavior here?
> 
> Looks like we should probably switch to Number.MAX_SAFE_INTEGER for that
> const. I copied that line from the cookie code in sessionstore that has been
> around since Firefox 2. I'm guessing that the author meant to actually use
> 2^53.

Mike, can someone please get this fixed? Not sure how critical that is, but maybe it might be a good mentored bug?

Comment 1

[Mike de Boer [:mikedeboer]]

OK, sure. I also read "[...]the integer 2^53 + 1 can't be directly represented in IEEE-754 but instead rounds to 2^53 under round-to-nearest and round-to-zero rounding."[1]; so I think when this value is normalized, it'll also be rounded to 2^53 using that same principle. Is there still an issue, then?

Regardless, I think it'd be best to use `Number.MAX_SAFE_INTEGER` of course.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isSafeInteger

Comment 2

[nicole_byer]

Hi, I’m a recent CS graduate and would like to start contributing. Would I be able to take care of this?
Thanks!
Nicole Byer

Comment 3

[Mike de Boer [:mikedeboer]]

Hi Nicole, thanks for reaching out and wanting to help out!

You can follow the steps outlined in this document: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction and get a working build of Firefox. This time you'll be working in frontend Javascript code, so you can use the faster artifact mode to build Firefox.

Then you'll want to open the file browser/components/sessionstore/SessionCookies.jsm in your favorite code editor and change the lines at https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionCookies.jsm#20-21.

Specifically, you'll need to change the Max.pow() statement to use Number.MAX_SAFE_INTEGER instead.

If you've got any further questions, please feel free to reach out to me (or others) here or on IRC (channels #introduction and #developers).

Comment 4

[Nicole Byer]

Hi Mike,
I got a working build of Firefox and changed the line of code (I'll attach the file). What is the next step? Do I need to test this?
Thanks for your help!
Nicole

Comment 5

[Nicole Byer]

Attachment: SessionCookies.jsm

Lines 20-21 changed to use Number.MAX_SAFE_INTEGER

Comment 6

[Mike de Boer [:mikedeboer]]

Hi Nicole, great that you've been able to get a build running! Next step is to submit a patch for review. You can read all about that here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

This is step 4 in the Contributing tutorial at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction.

Comment 7

[Nicole Byer]

Ok, great! Are you the reviewer? (I also see ttaubert when I run hg annotate). Thanks!

Comment 8

[Mike de Boer [:mikedeboer]]

Yeah, I'm the module owner, so I can review your patch.

Comment 9

[Nicole Byer]

Attachment: SessionCookies.jsm

Changed lines 20-21 to use Number.MAX_SAFE_INTEGER

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8921478 [details] [diff] [review]
SessionCookies.jsm

Nicole, the file you attached is not a patch, but the whole SessionCookies.jsm file, which is something we can't really review.
We always review sets of changes (i.e. a patch), which is quite a bit smaller and doesn't require you to point out what you've changed and where you changed something. Instead you can focus on explaining why you made the change in a commit message.

I really recommend you to give https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch a thorough read-through to see how you can generate a patch.

Comment 11

[Nicole Byer]

Attachment: Proposed patch

Hi,
Sorry about that; I hope this is more along the right lines!

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8921544 [details] [diff] [review]
Proposed patch

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

Perfect! Thanks for this :)

Comment 13

[Nicole Byer]

Great, thanks for your help!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a2659e778a
Changed use of 2^62 to  Number.MAX_SAFE_INTEGER (represents 2^53 - 1) to avoid faulty behavior. r=mikedeboer

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/44a2659e778a