Bug 235236 – Make LocalStorage prewarming async

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 449186 [details]
Patch

[Sihui Liu]

Created attachment 449194 [details]
Patch

[Sihui Liu]

Created attachment 449300 [details]
Patch

[Sihui Liu]

Created attachment 449344 [details]
Patch

[Sihui Liu]

Created attachment 449346 [details]
Patch

[Sihui Liu]

Created attachment 449351 [details]
Patch

[Chris Dumez]

Comment on attachment 449351 [details]
Patch

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

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:35
> +    static std::atomic<uint64_t> currenIdentifier(0);

(0) is unnecessary.

Does this really get called from different threads? I see that you're using atomic here and generateThreadSafe() below.

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:36
> +    return ++currenIdentifier;

typo: current

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:50
> +    ASSERT(!m_listeners.contains(connection) || m_listeners.get(connection) == identifier);

Could you clarify why this new assertion is true?

A connection identifies a WebProcess. What prevents having several StorageAreaMapIdentifier within a single WebProcess (especially when several tabs share the same WebProcess)?

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:82
> +    HashMap<IPC::Connection::UniqueID, StorageAreaMapIdentifier> m_listeners;

I am surprised we can have a map from IPC::Connection/WebProcess to a single StorageAreaMapIdentifier.

> Source/WebKit/WebProcess/WebProcess.cpp:1192
> +    for (auto key : copyToVector(m_storageAreaMaps.keys()))

The fact that we call copyToVector() here seems to indicate that m_storageAreaMaps can change while we iterate.

> Source/WebKit/WebProcess/WebProcess.cpp:1193
> +        m_storageAreaMaps.get(key)->disconnect();

however, you fail to null check the value returned by m_storageAreaMaps.get(key) here so this doesn't look safe?

> Source/WebKit/WebProcess/WebProcess.cpp:1702
> +    m_storageAreaMaps.add(identifier, storageAreaMap);

Here, the code seems to indicate we can have several StorageAreaMapIdentifiers within a single WebProcess.

[Sihui Liu]

Comment on attachment 449351 [details]
Patch

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

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:35
>> +    static std::atomic<uint64_t> currenIdentifier(0);
> 
> (0) is unnecessary.
> 
> Does this really get called from different threads? I see that you're using atomic here and generateThreadSafe() below.

Will remove. Each session has a storage thread.

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:36
>> +    return ++currenIdentifier;
> 
> typo: current

oh, will change.

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:50
>> +    ASSERT(!m_listeners.contains(connection) || m_listeners.get(connection) == identifier);
> 
> Could you clarify why this new assertion is true?
> 
> A connection identifies a WebProcess. What prevents having several StorageAreaMapIdentifier within a single WebProcess (especially when several tabs share the same WebProcess)?

Nothing prevents several StorageAreaMaps in a web process, we just don't have multiple StorageAreaMaps in a web process pointing to the same StorageArea... 
So previously StorageArea should receive only one sync connect message from one web process, but now it may receives two: one async and one sync. One the second sync message, the listener (IPC::Connection) should already be added, and in this case the message should come from the same StorageAreaMap in the process.

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:82
>> +    HashMap<IPC::Connection::UniqueID, StorageAreaMapIdentifier> m_listeners;
> 
> I am surprised we can have a map from IPC::Connection/WebProcess to a single StorageAreaMapIdentifier.

In web process, a StorageAreaMap can be identified by {session, origin, storageType} or StorageAreaMapIdentifier.
In network process, a StorageArea can be identified by {session, origin, storageType} or StorageAreaIdentifier.
Therefore, there should not be two StorageAreaMaps in a web process pointing to the same StorageArea in a web process (a StorageArea should not record two StorageAreaMapIdentifiers for one IPC::Connection.)

>> Source/WebKit/WebProcess/WebProcess.cpp:1193
>> +        m_storageAreaMaps.get(key)->disconnect();
> 
> however, you fail to null check the value returned by m_storageAreaMaps.get(key) here so this doesn't look safe?

Yes, should add a null check.

>> Source/WebKit/WebProcess/WebProcess.cpp:1702
>> +    m_storageAreaMaps.add(identifier, storageAreaMap);
> 
> Here, the code seems to indicate we can have several StorageAreaMapIdentifiers within a single WebProcess.

That's true a web process can have multiple StorageAreaMapIdentifiers, that's why StorageArea(network process) needs to send back StorageAreaMapIdentifier in the messages to let web process know which StorageAreaMap has change...

[Sihui Liu]

Created attachment 449441 [details]
Patch

[Sihui Liu]

Created attachment 449442 [details]
Patch

[Sihui Liu]

Created attachment 449443 [details]
Patch

[Sihui Liu]

Created attachment 449444 [details]
Patch

[Sihui Liu]

Created attachment 449447 [details]
Patch

[Sihui Liu]

Created attachment 449449 [details]
Patch

[Chris Dumez]

Comment on attachment 449449 [details]
Patch

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

r=me with nit. Should probably monitor PLT bots after this lands given that this changes a PLT optimization.

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:52
> +    m_listeners.set(connection, identifier);

Why set() and not add()? add() is more efficient and based on your assertions above, it seems add() would suffice.

[Sihui Liu]

Created attachment 449579 [details]
Patch for landing

[Sihui Liu]

(In reply to Chris Dumez from comment #15)
> Comment on attachment 449449 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449449&action=review
> 
> r=me with nit. Should probably monitor PLT bots after this lands given that
> this changes a PLT optimization.

Sure

> 
> > Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:52
> > +    m_listeners.set(connection, identifier);
> 
> Why set() and not add()? add() is more efficient and based on your
> assertions above, it seems add() would suffice.

Changed to use add().

[EWS]

Committed r288298 (246219@main): <https://commits.webkit.org/246219@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/87833142>