Bug 226295 – Abandon pending tasks on background thread when WebIDBServer is closed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Sihui Liu]

Potential fix for rdar://77532291.

[Sihui Liu]

Created attachment 429839 [details]
Patch

[Chris Dumez]

Comment on attachment 429839 [details]
Patch

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

> Source/WTF/ChangeLog:8
> +        Make the wait ends with either a new message or queue being killed.

Ends -> end

> Source/WebKit/ChangeLog:9
> +        tasks. We can ditch them by killing the CrossThreadQueue. For ongoing task, it can be blockd on quota check, so

Typo: blockd

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:425
> +        callOnMainRunLoop([protectedThis = WTFMove(protectedThis)]() mutable { });

Do we really need the mutable?

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

Is it ok to call the close callback before we’ve actually asynchronously done it?

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:101
> +    std::unique_ptr<WebCore::IDBServer::IDBServer> m_server WTF_GUARDED_BY_LOCK(m_serverLock);

Nice :)

[Sihui Liu]

Comment on attachment 429839 [details]
Patch

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

>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:425
>> +        callOnMainRunLoop([protectedThis = WTFMove(protectedThis)]() mutable { });
> 
> Do we really need the mutable?

Nope

>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:436
>>      m_closeCallback();
> 
> Is it ok to call the close callback before we’ve actually asynchronously done it?

Yes, this currently is just removing itself from the map of Network process and we keep the reference with CompletionCallback above.

[Sihui Liu]

Created attachment 429848 [details]
Patch

[Sihui Liu]

Created attachment 429883 [details]
Patch

[Chris Dumez]

Comment on attachment 429883 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        abort transactions on the main thread.

Why is it safe then? There is no guarantee that SQLite is built with thread-safety support enabled or that SQLite mutexes are fully enabled at runtime.

[Sihui Liu]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 429883 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429883&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        abort transactions on the main thread.
> 
> Why is it safe then? There is no guarantee that SQLite is built with
> thread-safety support enabled or that SQLite mutexes are fully enabled at
> runtime.

Tasks on the background thread need to acquire the lock for m_server to perform database operations. They acquire the lock at start, release the lock before performing quota check (which may be blocked on waiting user permission), and acquire the lock again after it.

[Sihui Liu]

Created attachment 429892 [details]
Patch

[Chris Dumez]

Comment on attachment 429892 [details]
Patch

r=me

[EWS]

Committed r278179 (238222@main): <https://commits.webkit.org/238222@main>

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