Bug 171367 – Refactor SessionID to support multiple non-ephemeral (persistent) sessions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Refactor SessionID to support multiple persistent sessions

[Brady Eidson]

Created attachment 308352 [details]
Patch

[Brady Eidson]

gtk == Forgot to add the new cpp file to cmake, of course.

Still reviewable, would fix before landing.

[Alex Christensen]

Comment on attachment 308352 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308352&action=review

> Source/WebCore/page/SessionID.cpp:37
> +static uint64_t currentPersistentID = 1;

I don't think this will work.  Other processes will also need to be able to tell whether this session is ephemeral or not, and their static variables won't be synchronized.  We should probably make a SessionID a wrapper around a bool and a uint64_t.

[Brady Eidson]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 308352 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308352&action=review
> 
> > Source/WebCore/page/SessionID.cpp:37
> > +static uint64_t currentPersistentID = 1;
> 
> I don't think this will work.  Other processes will also need to be able to
> tell whether this session is ephemeral or not, and their static variables
> won't be synchronized.  We should probably make a SessionID a wrapper around
> a bool and a uint64_t.

Yup, of course it won't, duh.

[Brady Eidson]

(In reply to Alex Christensen from comment #3)

> We should probably make a SessionID a wrapper around a bool and a uint64_t.

Going to leave it a uint64_t, reserving the high bit for the bool.

[Brady Eidson]

Created attachment 308406 [details]
Patch

[Andy Estes]

Comment on attachment 308406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308406&action=review

> Source/WebCore/page/SessionID.cpp:35
> +static bool generationProtectionEnabled = false;

No need for the = false.

> Source/WebCore/page/SessionID.h:90
> +    return sessionID.isValid();

Is an invalid sessionID really an decoding failure? Does something stop us from encoding invalid values?

[Alex Christensen]

Comment on attachment 308406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308406&action=review

> Source/WebCore/page/SessionID.cpp:34
> +static uint64_t currentEphemeralID = SessionID::LegacyPrivateSessionID;

Couldn't we just start at std::numeric_limits<uint64_t>::max() and count backwards?

> Source/WebCore/page/SessionID.h:40
> +    enum {

Don't we like enum classes these days?  Also, does this need to be public?

[Brady Eidson]

(In reply to Alex Christensen from comment #8)
> Comment on attachment 308406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308406&action=review
> 
> > Source/WebCore/page/SessionID.cpp:34
> > +static uint64_t currentEphemeralID = SessionID::LegacyPrivateSessionID;
> 
> Couldn't we just start at std::numeric_limits<uint64_t>::max() and count
> backwards?

Could we? Yes.

But that would be a magic constant with no explanation.

Makes sense to me to have the persistent and ephemeral IDs work the same way - Start at the shared default for each and then count up.

> 
> > Source/WebCore/page/SessionID.h:40
> > +    enum {
> 
> Don't we like enum classes these days?  Also, does this need to be public?

We do like enum classes with standalone types but are not adverse to enums inside of classes.

Also, the purposes of these enums are not for type safety, but rather to capture some magic constants. If it were an enum class there'd be a lot of unnecessary casting-to-uint64_t literally everywhere they were used.

[Brady Eidson]

(In reply to Alex Christensen from comment #8)
> Comment on attachment 308406 [details]
> 
> > Source/WebCore/page/SessionID.h:40
> > +    enum {
> Also, does this need to be public?

It does not need to be, but it does need to be declared before the 4 static c'tors that use them.

I'll add a second private section for the enums.

[Brady Eidson]

(In reply to Andy Estes from comment #7)
> Comment on attachment 308406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308406&action=review
> 
> > Source/WebCore/page/SessionID.cpp:35
> > +static bool generationProtectionEnabled = false;
> 
> No need for the = false.

Indeed.

> 
> > Source/WebCore/page/SessionID.h:90
> > +    return sessionID.isValid();
> 
> Is an invalid sessionID really an decoding failure? Does something stop us
> from encoding invalid values?

I think so.

The invalid ones should only be used for Hashing, and even if you were to encode a HashSet/HashMap with these in it you'd never actually encode/decode invalid sessionIDs.

[Brady Eidson]

Created attachment 308411 [details]
Patch for landing

Will CQ+ after EWS and local API tests run.

[Brady Eidson]

(In reply to Brady Eidson from comment #10)
> (In reply to Alex Christensen from comment #8)
> > Comment on attachment 308406 [details]
> > 
> > > Source/WebCore/page/SessionID.h:40
> > > +    enum {
> > Also, does this need to be public?
> 
> It does not need to be, but it does need to be declared before the 4 static
> c'tors that use them.
> 
> I'll add a second private section for the enums.

Actually yes, they need to be public.

[Brady Eidson]

Created attachment 308414 [details]
Patch for landing

[Build Bot]

Comment on attachment 308414 [details]
Patch for landing

Attachment 308414 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3621386

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 308429 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Brady Eidson]

That "&" should be a "|"

:)

(Rebuilding the world now...)

[Build Bot]

Comment on attachment 308414 [details]
Patch for landing

Attachment 308414 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3621314

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 308430 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308414 [details]
Patch for landing

Attachment 308414 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3621439

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 308431 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Brady Eidson]

Created attachment 308435 [details]
Patch for EWS

[Brady Eidson]

(In reply to Andy Estes from comment #7)
> 
> > Source/WebCore/page/SessionID.h:90
> > +    return sessionID.isValid();
> 
> Is an invalid sessionID really an decoding failure? Does something stop us
> from encoding invalid values?

Unfortunately (or perhaps fortunately) this came up in a single API test.

I'm replacing the ASSERTs with FIXMEs for now, as I still think it's the right way to go.

[Brady Eidson]

Created attachment 308445 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 308445 [details]
Patch for landing

Clearing flags on attachment: 308445

Committed r215907: <http://trac.webkit.org/changeset/215907>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.