Bug 171367

Summary: Refactor SessionID to support multiple non-ephemeral (persistent) sessions
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, buildbot, commit-queue, rniwa, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
achristensen: review-, beidson: commit-queue-
Patch
aestes: review+, aestes: commit-queue-
Patch for landing
none
Patch for landing
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Patch for EWS
none
Patch for landing none

Description Brady Eidson 2017-04-26 23:22:12 PDT
Refactor SessionID to support multiple persistent sessions
Comment 1 Brady Eidson 2017-04-27 00:02:20 PDT
Created attachment 308352 [details]
Patch
Comment 2 Brady Eidson 2017-04-27 00:23:31 PDT
gtk == Forgot to add the new cpp file to cmake, of course.

Still reviewable, would fix before landing.
Comment 3 Alex Christensen 2017-04-27 00:40:24 PDT
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.
Comment 4 Brady Eidson 2017-04-27 08:58:03 PDT
(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.
Comment 5 Brady Eidson 2017-04-27 09:35:16 PDT
(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.
Comment 6 Brady Eidson 2017-04-27 10:25:18 PDT
Created attachment 308406 [details]
Patch
Comment 7 Andy Estes 2017-04-27 10:30:51 PDT
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?
Comment 8 Alex Christensen 2017-04-27 10:37:48 PDT
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?
Comment 9 Brady Eidson 2017-04-27 10:50:11 PDT
(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.
Comment 10 Brady Eidson 2017-04-27 10:51:04 PDT
(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.
Comment 11 Brady Eidson 2017-04-27 10:53:05 PDT
(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.
Comment 12 Brady Eidson 2017-04-27 11:15:18 PDT
Created attachment 308411 [details]
Patch for landing

Will CQ+ after EWS and local API tests run.
Comment 13 Brady Eidson 2017-04-27 11:24:25 PDT
(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.
Comment 14 Brady Eidson 2017-04-27 11:25:30 PDT
Created attachment 308414 [details]
Patch for landing
Comment 15 Build Bot 2017-04-27 12:34:43 PDT
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.
Comment 16 Build Bot 2017-04-27 12:34:44 PDT
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
Comment 17 Brady Eidson 2017-04-27 12:36:09 PDT
That "&" should be a "|"

:)

(Rebuilding the world now...)
Comment 18 Build Bot 2017-04-27 12:38:47 PDT
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.
Comment 19 Build Bot 2017-04-27 12:38:48 PDT
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
Comment 20 Build Bot 2017-04-27 12:45:32 PDT
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.
Comment 21 Build Bot 2017-04-27 12:45:34 PDT
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
Comment 22 Brady Eidson 2017-04-27 13:09:22 PDT
Created attachment 308435 [details]
Patch for EWS
Comment 23 Brady Eidson 2017-04-27 14:07:13 PDT
(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.
Comment 24 Brady Eidson 2017-04-27 14:18:10 PDT
Created attachment 308445 [details]
Patch for landing
Comment 25 WebKit Commit Bot 2017-04-27 16:48:54 PDT
Comment on attachment 308445 [details]
Patch for landing

Clearing flags on attachment: 308445

Committed r215907: <http://trac.webkit.org/changeset/215907>
Comment 26 WebKit Commit Bot 2017-04-27 16:48:56 PDT
All reviewed patches have been landed.  Closing bug.