Bug 225658 – Use one VM per thread for IDB serialization work

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Sihui Liu]

...

[Sihui Liu]

Created attachment 428289 [details]
Patch

[Sihui Liu]

<rdar://77722187>

[Chris Dumez]

Comment on attachment 428289 [details]
Patch

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

r=me with suggestions.

> Source/WebCore/ChangeLog:11
> +        does not stay around, and WebIDBServer will be destroyed after it finishes schedueld tasks on the background 

Typo: schedueld

> Source/WebCore/ChangeLog:12
> +        thread.Then, it's possible that while a WebIDBServer for some session is removed and finishing last tasks, 

Typo: .Then

> Source/WebCore/ChangeLog:18
> +        making the map in IDBSerializationContext keye by thread pointer.

Typo: keye (I guess you meant keyed?)

> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.cpp:39
> +static HashMap<Thread*, IDBSerializationContext*>& serializationContextMap()

It would look safer if this function took a Locker<Lock>& as parameter, since the caller needs to grab the lock.

> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.h:43
> +    static Ref<IDBSerializationContext> getOrCreateIDBSerializationContext();

Maybe we could rename this to getOrCreateForCurrentThread() for clarity, given the new behavior? (note that I don't think we need to repeat "IDBSerializationContext" in the function name since it is already in the IDBSerializationContext class scope.

> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.h:51
> +    IDBSerializationContext(Thread&);

explicit

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:45
> +static RetainPtr<WKScriptMessage> lastScriptMessage;

Maybe this could be a RetainPtr<NSString> instead.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:76
> +    lastScriptMessage = message;

Maybe we could store [message body] here. Doesn't seem we really want the whole message.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:112
> +    RetainPtr<DatabaseProcessKillMessageHandler> handler = adoptNS([[DatabaseProcessKillMessageHandler alloc] init]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:113
> +    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:116
> +    auto ephemeralStore = [WKWebsiteDataStore nonPersistentDataStore];

This local variable is not needed.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:125
> +            var request = window.indexedDB.open('testDB'); \

can omit "window."

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:129
> +                for (let i = 0; i < 10000; i ++) \

extra space between i and ++

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:131
> +                window.webkit.messageHandlers.testHandler.postMessage('Opened');\

can omit "window."

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:140
> +    [webView evaluateJavaScript:@"openDatabase()" completionHandler: nil];

no space after completionHandler:

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:142
> +    RetainPtr<NSString> string = (NSString *)[lastScriptMessage body];

Local variable would not be needed with my suggestion above.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:147
> +    [secondWebView evaluateJavaScript:@"openDatabase()" completionHandler: nil];

no space after completionHandler:

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:148
> +    receivedScriptMessage = false;

It would look safer if we did this before the evaluateJavaScript call (although it is safe because async).

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:150
> +    string = (NSString *)[lastScriptMessage body];

Local variable would not be needed with my suggestion above.

[Sihui Liu]

Comment on attachment 428289 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/ChangeLog:18
>> +        making the map in IDBSerializationContext keye by thread pointer.
> 
> Typo: keye (I guess you meant keyed?)

Yes.

>> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.cpp:39
>> +static HashMap<Thread*, IDBSerializationContext*>& serializationContextMap()
> 
> It would look safer if this function took a Locker<Lock>& as parameter, since the caller needs to grab the lock.

Will add.

>> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.h:43
>> +    static Ref<IDBSerializationContext> getOrCreateIDBSerializationContext();
> 
> Maybe we could rename this to getOrCreateForCurrentThread() for clarity, given the new behavior? (note that I don't think we need to repeat "IDBSerializationContext" in the function name since it is already in the IDBSerializationContext class scope.

Sure.

>> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.h:51
>> +    IDBSerializationContext(Thread&);
> 
> explicit

Will add.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:45
>> +static RetainPtr<WKScriptMessage> lastScriptMessage;
> 
> Maybe this could be a RetainPtr<NSString> instead.

Sure

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:76
>> +    lastScriptMessage = message;
> 
> Maybe we could store [message body] here. Doesn't seem we really want the whole message.

Sure, will change.

[Sihui Liu]

Created attachment 428323 [details]
Patch for landing

[EWS]

Committed r277343 (237601@main): <https://commits.webkit.org/237601@main>

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