Bug 204346 – [IndexedDB] IndexedDB's threading assertion should respect Web thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[IndexedDB] IndexedDB's threading assertion should respect Web thread

[Yusuke Suzuki]

Created attachment 383854 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/57299449>

[Yusuke Suzuki]

Created attachment 383855 [details]
Patch

[Daniel Bates]

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&().

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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`

[Yusuke Suzuki]

Committed r252642: <https://trac.webkit.org/changeset/252642>

[Yusuke Suzuki]

Committed r252651: <https://trac.webkit.org/changeset/252651>