WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(106.72 KB, patch)
2019-11-19 03:14 PST
,
Yusuke Suzuki
dbates
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2019-11-19 03:10:15 PST
Created
attachment 383854
[details]
Patch
Yusuke Suzuki
Comment 2
2019-11-19 03:11:13 PST
<
rdar://problem/57299449
>
Yusuke Suzuki
Comment 3
2019-11-19 03:14:06 PST
Created
attachment 383855
[details]
Patch
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
Committed
r252642
: <
https://trac.webkit.org/changeset/252642
>
Yusuke Suzuki
Comment 8
2019-11-19 14:31:16 PST
Committed
r252651
: <
https://trac.webkit.org/changeset/252651
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug