Bug 236844 – Add assertion that no two sessions share the same general storage directory

• Status: RESOLVED
• Resolution: FIXED
• Component: Website Storage

[Sihui Liu]

...

[Sihui Liu]

Created attachment 452539 [details]
Patch

[Sihui Liu]

Created attachment 452586 [details]
Patch

[Chris Dumez]

Comment on attachment 452586 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:3
> +        Add assertion that no two network session share the same storage path

sessions (plural)

> Source/WebKit/ChangeLog:8
> +        Each NetworkStorageManager has its own queue and access the storage files on that queue, so we can't have two

accesses

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:115
> +    static NeverDestroyed<HashSet<String>> paths;

I just learned today that you can use MainThreadNeverDestroyed (and drop your main thread assertion above).

[Sihui Liu]

Created attachment 452609 [details]
Patch for landing

[EWS]

Committed r290196 (247524@main): <https://commits.webkit.org/247524@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452609 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89178566>

[Chris Dumez]

Reverted r290196 for reason:

Caused Network Process crashes <rdar://89190571>

Committed r290214 (247542@trunk): <https://commits.webkit.org/247542@trunk>

[Sihui Liu]

Created attachment 453443 [details]
Patch

[Chris Dumez]

Comment on attachment 453443 [details]
Patch

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

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:211
> +    HashMap<String, PAL::SessionID> directoriesBySession;

This name seems wrong. If something says "BySession", then I'd expect the session to be the HashMap key, not the value.
Maybe this could be named something like: "directorySessions" or "sessionForDirectory"

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:216
> +                RELEASE_ASSERT_WITH_MESSAGE(directoriesBySession.add(directory, sessionID).isNewEntry, "GeneralStorageDirectory for session '%llu' is already in use by session '%llu'", directoriesBySession.get(directory).toUInt64(), sessionID.toUInt64());

We're supposed to use PRIu64, not %llu to print 64bit integers to be friendly to Linux ports. Looks like this code may not build on Linux but I think it I still good practice.

[Sihui Liu]

Created attachment 453446 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 453443 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453443&action=review
> 
> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:211
> > +    HashMap<String, PAL::SessionID> directoriesBySession;
> 
> This name seems wrong. If something says "BySession", then I'd expect the
> session to be the HashMap key, not the value.
> Maybe this could be named something like: "directorySessions" or
> "sessionForDirectory"

Changed to sessionForDirectory.

> 
> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:216
> > +                RELEASE_ASSERT_WITH_MESSAGE(directoriesBySession.add(directory, sessionID).isNewEntry, "GeneralStorageDirectory for session '%llu' is already in use by session '%llu'", directoriesBySession.get(directory).toUInt64(), sessionID.toUInt64());
> 
> We're supposed to use PRIu64, not %llu to print 64bit integers to be
> friendly to Linux ports. Looks like this code may not build on Linux but I
> think it I still good practice.

Sure, changed to PRIu64.

[Sihui Liu]

Created attachment 453563 [details]
Patch

[Sihui Liu]

Created attachment 453573 [details]
Patch

[EWS]

ChangeLog entry in Tools/ChangeLog contains OOPS!.

[Sihui Liu]

Created attachment 453627 [details]
Patch for landing

[EWS]

Committed r290739 (247985@main): <https://commits.webkit.org/247985@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453627 [details].