RESOLVED FIXED 204346
[IndexedDB] IndexedDB's threading assertion should respect Web thread
https://bugs.webkit.org/show_bug.cgi?id=204346
Summary [IndexedDB] IndexedDB's threading assertion should respect Web thread
Yusuke Suzuki
Reported 2019-11-19 03:07:00 PST
[IndexedDB] IndexedDB's threading assertion should respect Web thread
Attachments
Patch (106.69 KB, patch)
2019-11-19 03:10 PST, Yusuke Suzuki
no flags
Patch (106.72 KB, patch)
2019-11-19 03:14 PST, Yusuke Suzuki
dbates: review+
Yusuke Suzuki
Comment 1 2019-11-19 03:10:15 PST
Yusuke Suzuki
Comment 2 2019-11-19 03:11:13 PST
Yusuke Suzuki
Comment 3 2019-11-19 03:14:06 PST
Daniel Bates
Comment 4 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&().
Yusuke Suzuki
Comment 5 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.
Yusuke Suzuki
Comment 6 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`
Yusuke Suzuki
Comment 7 2019-11-19 12:53:41 PST
Yusuke Suzuki
Comment 8 2019-11-19 14:31:16 PST
Note You need to log in before you can comment on or make changes to this bug.