Bug 204346 - [IndexedDB] IndexedDB's threading assertion should respect Web thread
Summary: [IndexedDB] IndexedDB's threading assertion should respect Web thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-19 03:07 PST by Yusuke Suzuki
Modified: 2019-11-19 14:31 PST (History)
7 users (show)

See Also:


Attachments
Patch (106.69 KB, patch)
2019-11-19 03:10 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (106.72 KB, patch)
2019-11-19 03:14 PST, Yusuke Suzuki
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-11-19 03:07:00 PST
[IndexedDB] IndexedDB's threading assertion should respect Web thread
Comment 1 Yusuke Suzuki 2019-11-19 03:10:15 PST
Created attachment 383854 [details]
Patch
Comment 2 Yusuke Suzuki 2019-11-19 03:11:13 PST
<rdar://problem/57299449>
Comment 3 Yusuke Suzuki 2019-11-19 03:14:06 PST
Created attachment 383855 [details]
Patch
Comment 4 Daniel Bates 2019-11-19 11:47:45 PST
Comment on attachment 383855 [details]
Patch

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

This patch looks good.

> Source/WebCore/Modules/indexeddb/IDBActiveDOMObject.h:41
> +        ASSERT(canAccessThreadLocalDataForThread(m_originThread.get()));

Naming is ok-as is. The slightly less verbose name would be "canAccessThreadLocalData". That's no need for "ForThread" as that can be implied by the passed argument.

> Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:54
> +        ASSERT(canAccessThreadLocalDataForThread(m_originThread.get()));

This is ok as-is. The .get() is unnecessary because Ref<> overloads operator T&().
Comment 5 Yusuke Suzuki 2019-11-19 12:01:09 PST
Comment on attachment 383855 [details]
Patch

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

Thanks!

>> Source/WebCore/Modules/indexeddb/IDBActiveDOMObject.h:41
>> +        ASSERT(canAccessThreadLocalDataForThread(m_originThread.get()));
> 
> Naming is ok-as is. The slightly less verbose name would be "canAccessThreadLocalData". That's no need for "ForThread" as that can be implied by the passed argument.

I think this ForThread is a bit important since this function is returning true when the argument’s thread data can be accessed by the current thread. Without “ForThread”, the function name does not tell wether it is (1) checking access right from the current thread to the argument thread or (2) checking the access right from the argument thread to the current thread.

>> Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:54
>> +        ASSERT(canAccessThreadLocalDataForThread(m_originThread.get()));
> 
> This is ok as-is. The .get() is unnecessary because Ref<> overloads operator T&().

Thanks, will fix.
Comment 6 Yusuke Suzuki 2019-11-19 12:46:29 PST
Comment on attachment 383855 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/IDBActiveDOMObject.h:41
>>> +        ASSERT(canAccessThreadLocalDataForThread(m_originThread.get()));
>> 
>> Naming is ok-as is. The slightly less verbose name would be "canAccessThreadLocalData". That's no need for "ForThread" as that can be implied by the passed argument.
> 
> I think this ForThread is a bit important since this function is returning true when the argument’s thread data can be accessed by the current thread. Without “ForThread”, the function name does not tell wether it is (1) checking access right from the current thread to the argument thread or (2) checking the access right from the argument thread to the current thread.

Talked with Daniel, we should rename it to `canCurrentThreadAccessThreadLocalData`
Comment 7 Yusuke Suzuki 2019-11-19 12:53:41 PST
Committed r252642: <https://trac.webkit.org/changeset/252642>
Comment 8 Yusuke Suzuki 2019-11-19 14:31:16 PST
Committed r252651: <https://trac.webkit.org/changeset/252651>