Bug 140881 – [WK2] Replace AsyncTask with C++ lambdas

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Zan Dobersek]

[WK2] Replace AsyncTask with C++ lambdas

[Zan Dobersek]

Created attachment 245332 [details]
WIP

[WebKit Commit Bot]

Attachment 245332 [details] did not pass style-queue:


ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:352:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1349:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1371:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1388:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1414:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:108:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:122:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:139:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:205:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:208:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/WebCore.exp.in:0:  Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script  [list/order] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:154:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:171:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 245332 [details]
WIP

Is this really wip? what's missing? EWS is green for all ports.

[Carlos Garcia Campos]

Darin, Anders, what do you think about this? it makes the code simpler and more compatible with the compilers.

[Zan Dobersek]

Created attachment 245638 [details]
Patch

[WebKit Commit Bot]

Attachment 245638 [details] did not pass style-queue:


ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:352:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1349:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1371:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1388:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1414:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:110:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:122:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:139:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:205:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:208:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/WebCore.exp.in:0:  Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script  [list/order] [5]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:154:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:171:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 245638 [details]
Patch

I prefer this style; proofreading all the call sites to makes sure they are all correct is a bit of a challenge.

[Carlos Garcia Campos]

(In reply to comment #7)
> Comment on attachment 245638 [details]
> Patch
> 
> I prefer this style; proofreading all the call sites to makes sure they are
> all correct is a bit of a challenge.

I have reviewed them and everything looks correct. Mac EWS would have caught any issue anyway, I guess.

[Darin Adler]

Comment on attachment 245638 [details]
Patch

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

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:305
> +    IDBIdentifier capturedIdentifier(transactionIdentifier.isolatedCopy());

This idiom isn’t great. right now, the isolatedCopy function does nothing special. If it did, we still might have a race condition on deref like the one we have in String, so isolatedCopy might be insufficient. I’m not sure it’s all that helpful to write the code this way instead of just using straight construction; it’s not really future proof to have a function need isolatedCopy.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:416
> +    IDBIdentifier capturedIdentifier = transactionIdentifier.isolatedCopy();

Strange that this uses "=" syntax where the other call sites use construction syntax. I don’t have a strong preference, but I do prefer consistency.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:650
> +    IDBObjectStoreMetadata capturedMetadata(m_metadata->objectStores.get(objectStoreID).isolatedCopy());

This won’t work. We have a race condition where the main thread will deref capturedMetadata.name and the database task thread will also deref it. And String does not have a thread-safe reference count.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:651
> +    IDBKeyData capturedKeyData(keyData.isolatedCopy());

Same problem here with capturedKeyData.stringValue.

[Carlos Garcia Campos]

This no longer blocks #98932 , since we bumped GCC requirements in the end, but I think it would still be desirable if we manage to fix the thread-safety issues

[Zan Dobersek]

AsyncTask doesn't exist anymore.