Bug 224305 – Create WebIDBServer only when it is needed

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

rdar://71962196

[Sihui Liu]

Created attachment 425445 [details]
Patch

[Sihui Liu]

Created attachment 425475 [details]
Patch

[Alex Christensen]

Comment on attachment 425475 [details]
Patch

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

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:70
> +        postTaskReply(CrossThreadTask([this, callback = WTFMove(callback), origins = crossThreadCopy(m_server->getOrigins())]() mutable {

Don't we need either a protected or a weak this here?

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:88
> +        postTaskReply(CrossThreadTask([this, callback = WTFMove(callback)]() mutable {

ditto

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:106
> +        postTaskReply(CrossThreadTask([this, callback = WTFMove(callback)]() mutable {

ditto

[Sihui Liu]

Created attachment 425521 [details]
Patch

[Sihui Liu]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 425475 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425475&action=review
> 
> > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:70
> > +        postTaskReply(CrossThreadTask([this, callback = WTFMove(callback), origins = crossThreadCopy(m_server->getOrigins())]() mutable {
> 
> Don't we need either a protected or a weak this here?
> 

Updated the patch.

By the way do you think revealing sessionID in thread name is a bad idea?

[Darin Adler]

Comment on attachment 425521 [details]
Patch

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

This looks good. I have some comments. I didn’t set review+ because I am not sure I’m enough of an expert on this area to spot mistakes, but I think it looks right.

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:97
> +    enum class DataTaskCountUpdateType : uint8_t { Decrease, Increase };
> +    void updateDataTaskCount(DataTaskCountUpdateType);

This is a peculiar interface. I suggest two separate named functions rather than a function with an enum class argument.

Also, to try to make this harder to get wrong, I suggest we make an object that increments on creation and decrements on destruction. We can capture that object in lambdas, instead of capturing "protectedThis" and writing an explicit decrement function.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2675
> +    if (auto* webIDBServer = m_webIDBServers.get(sessionID))
> +        webIDBServer->removeConnection(connection);

In a tight context like this, I really like single word variable names that don’t try to disambiguate or be precise. Like:

    if (auto server = m_webIDBServers.get(sessionID))
        server->removeConnection(connection);

Sure, the local variable can be named webIDBServer or even webIDBServerForSession, but it doesn’t need to be. Single words work really well to make such code clear and readable.

[Sihui Liu]

Created attachment 425542 [details]
Patch

[Sihui Liu]

(In reply to Darin Adler from comment #7)
> Comment on attachment 425521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425521&action=review
> 
> This looks good. I have some comments. I didn’t set review+ because I am not
> sure I’m enough of an expert on this area to spot mistakes, but I think it
> looks right.
> 
> > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:97
> > +    enum class DataTaskCountUpdateType : uint8_t { Decrease, Increase };
> > +    void updateDataTaskCount(DataTaskCountUpdateType);
> 
> This is a peculiar interface. I suggest two separate named functions rather
> than a function with an enum class argument.
> 
> Also, to try to make this harder to get wrong, I suggest we make an object
> that increments on creation and decrements on destruction. We can capture
> that object in lambdas, instead of capturing "protectedThis" and writing an
> explicit decrement function.
> 

Thanks for the comments. I've replaced it with a RefCounter.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2675
> > +    if (auto* webIDBServer = m_webIDBServers.get(sessionID))
> > +        webIDBServer->removeConnection(connection);
> 
> In a tight context like this, I really like single word variable names that
> don’t try to disambiguate or be precise. Like:
> 
>     if (auto server = m_webIDBServers.get(sessionID))
>         server->removeConnection(connection);
> 
> Sure, the local variable can be named webIDBServer or even
> webIDBServerForSession, but it doesn’t need to be. Single words work really
> well to make such code clear and readable.

Updated.

[Darin Adler]

Comment on attachment 425542 [details]
Patch

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

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:44
> +    , m_dataTaskCounter([protectedThis = makeRef(*this)](RefCounterEvent event) { protectedThis->tryClose(); })

This creates a reference cycle since the WebIDBServer owns its m_dataTaskCounter, which in turn owns this lambda, which in turn references the WebIDBServer, so I think the WebIDBServer will leak.

[Sihui Liu]

Created attachment 425555 [details]
Patch

[Alex Christensen]

Comment on attachment 425555 [details]
Patch

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

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:435
> +    m_closeCallback();

It would probably be worth null checking m_closeCallback to make this more robust in case some tries to call tryClose twice in the future.

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:107
> +    typedef RefCounter<DataTaskCounterType> DataTaskCounter;

"using DataTaskCounter = RefCounter<DataTaskCounterType>" is the cool new way to do this.  Same with DataTaskCounterToken

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2330
>          RefPtr<StorageQuotaManager> storageQuotaManager = weakThis ? this->storageQuotaManager(sessionID, origin) : nullptr;

If you used weakThis-> instead of this-> then you wouldn't need to capture this.  That would be better because it wouldn't allow you to accidentally use an unchecked this pointer (which is easy to do).

[Alex Christensen]

Comment on attachment 425555 [details]
Patch

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

> Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:62
> +    send(Messages::NetworkConnectionToWebProcess::AddIDBConnection());

Should there be a corresponding remove idb connection message in the destructor?

[Darin Adler]

Comment on attachment 425555 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2330
>>          RefPtr<StorageQuotaManager> storageQuotaManager = weakThis ? this->storageQuotaManager(sessionID, origin) : nullptr;
> 
> If you used weakThis-> instead of this-> then you wouldn't need to capture this.  That would be better because it wouldn't allow you to accidentally use an unchecked this pointer (which is easy to do).

This is excellent advice.

Also, when something is ThreadSafeRefCounted, we typically need to ref it before dereferencing it, not just dereference a weak pointer. There is likely a reason that’s not needed here, but *generally* the idiom would be to put the weak pointer into a RefPtr first.

[Sihui Liu]

Created attachment 425650 [details]
Patch for landing

[Sihui Liu]

Created attachment 425652 [details]
Patch for landing

[Sihui Liu]

Created attachment 425653 [details]
Patch for landing

[Sihui Liu]

Comment on attachment 425555 [details]
Patch

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

>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:435
>> +    m_closeCallback();
> 
> It would probably be worth null checking m_closeCallback to make this more robust in case some tries to call tryClose twice in the future.

Added.

>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:107
>> +    typedef RefCounter<DataTaskCounterType> DataTaskCounter;
> 
> "using DataTaskCounter = RefCounter<DataTaskCounterType>" is the cool new way to do this.  Same with DataTaskCounterToken

Cool.

>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2330
>>>          RefPtr<StorageQuotaManager> storageQuotaManager = weakThis ? this->storageQuotaManager(sessionID, origin) : nullptr;
>> 
>> If you used weakThis-> instead of this-> then you wouldn't need to capture this.  That would be better because it wouldn't allow you to accidentally use an unchecked this pointer (which is easy to do).
> 
> This is excellent advice.
> 
> Also, when something is ThreadSafeRefCounted, we typically need to ref it before dereferencing it, not just dereference a weak pointer. There is likely a reason that’s not needed here, but *generally* the idiom would be to put the weak pointer into a RefPtr first.

ah, I think we do need to ref it here. Updated in the patch.

>> Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:62
>> +    send(Messages::NetworkConnectionToWebProcess::AddIDBConnection());
> 
> Should there be a corresponding remove idb connection message in the destructor?

NetworkProcess::connectionToWebProcessClosed should already cover this case.

[EWS]

Committed r275779 (236352@main): <https://commits.webkit.org/236352@main>

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

[Sihui Liu]

Reopening to attach new patch.

[Sihui Liu]

Created attachment 425666 [details]
[fast-cq] Patch

[EWS]

Committed r275784 (236355@main): <https://commits.webkit.org/236355@main>

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

[Sihui Liu]

Reopening to attach new patch.

[Sihui Liu]

Created attachment 425680 [details]
[fast-cq] Patch

[EWS]

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

[Sihui Liu]

Created attachment 425681 [details]
[fast-cq] Patch

[EWS]

Committed r275794 (236365@main): <https://commits.webkit.org/236365@main>

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

[Sihui Liu]

Reopening to attach new patch.

[Sihui Liu]

Created attachment 425685 [details]
Patch for landing

[Sihui Liu]

Created attachment 425686 [details]
Patch for landing

[EWS]

Patch 425685 does not build

[EWS]

Committed r275799 (236370@main): <https://commits.webkit.org/236370@main>

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

[Sihui Liu]

Reopened as the patch is reverted in r275799

[Sihui Liu]

Created attachment 425748 [details]
Patch

[EWS]

Committed r275846 (236411@main): <https://commits.webkit.org/236411@main>

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

[Fujii Hironori]

Filed: Bug 225089 – HashTableConstIterator's consistency assertion fails while closing m_webIDBServers in NetworkProcess::didClose since r275846