Bug 230675 – Make StorageManager available in Worker

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 439022 [details]
Patch

[Sihui Liu]

Created attachment 439044 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83448115>

[Sihui Liu]

Created attachment 439151 [details]
Patch

[Sihui Liu]

Created attachment 439372 [details]
Patch

[Sihui Liu]

Created attachment 439374 [details]
Patch

[youenn fablet]

Comment on attachment 439374 [details]
Patch

Looks good to me overall, a few improvement ideas below.

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

> Source/WebCore/Modules/storage/StorageConnection.h:37
> +class StorageConnection : public ThreadSafeRefCounted<StorageConnection> {

I would mark it as DestructionThread::MainRunLoop for extra safety.

> Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:52
> +    callOnMainThreadAndWait([mainThreadConnection = WTFMove(m_mainThreadConnection)]() mutable { });

Not needed with DestructionThread::Main/MainRunLoop

> Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:66
> +    if (!m_scope)

I would add ASSERT(m_scope).

> Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:76
> +                downcast<WorkerStorageConnection>(*storageConnection).didGetPersisted(callbackIdentifier, result);

Ideally, we would not need that, WorkerGlobalScope could just return a WorkerStorageConnection, which would remove the need for isWorker().

> Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:91
> +    if (!m_scope)

Ditto here

> Source/WebCore/Modules/storage/WorkerStorageConnection.h:40
> +    void didGetPersisted(uint64_t callbackIdentifier, bool result);

Should be private

> Source/WebCore/Modules/storage/WorkerStorageConnection.h:43
> +    WorkerStorageConnection(WorkerGlobalScope&);

explicit

> Source/WebCore/Modules/storage/WorkerStorageConnection.h:50
> +    RefPtr<WorkerGlobalScope> m_scope;

WeakPtr to cut ref cycles?

> Source/WebCore/workers/WorkerGlobalScope.cpp:221
> +StorageConnection* WorkerGlobalScope::storageConnection()

Should be &, ideally WorkerStorageConnection&

> Source/WebCore/workers/WorkerGlobalScope.h:85
> +    StorageConnection* storageConnection() final;

Not sure whether we need a virtual storageConnection(), maybe we can have one for Document and one for WorkerGlobalScope, but not on ScriptExecutionContext.

[Sihui Liu]

Created attachment 439385 [details]
Patch

[Sihui Liu]

Created attachment 439396 [details]
Patch

[Sihui Liu]

Created attachment 439432 [details]
Patch

[youenn fablet]

Comment on attachment 439432 [details]
Patch

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

> Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:46
> +        m_mainThreadConnection = scope.thread().workerLoaderProxy().storageConnection();

storageConnection() is currently a pointer so m_mainThreadConnection can be nullptr for instance in getPersisted.
Maybe we can guarantee to have a reference here which will make sure m_mainThreadConnection is not null.

> Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:71
> +    callOnMainThread([callbackIdentifier, workerThread = Ref { m_scope->thread() }, mainThreadConnection = m_mainThreadConnection, origin = origin.isolatedCopy()]() mutable {

Since we are taking a ref to the worker thread, another strategy is to not have m_mainThreadConnection and retrieve it from worker thread loader proxy each time we need it from main thread.
I would tend to do this plus a null check on the connection.

> Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:72
> +        auto mainThreadCallback = [callbackIdentifier, workerThread = WTFMove(workerThread)](auto result) mutable {

Let's use bool here.
If in the future, we want to have ExceptionOr<bool>, we will need to change the code and maybe isolate copy it.

[Sihui Liu]

Created attachment 439492 [details]
Patch for landing

[EWS]

Committed r283184 (242232@main): <https://commits.webkit.org/242232@main>

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