Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2660463003:
CrossThreadPersistent should acquire a lock before assigning (Closed)
DescriptionCrossThreadPersistent should acquire a lock before assigning
The root set in CrossThreadPersistent should not change while a thread is trying to mark as it may cause objects unmarked when it should be marked.
This acquires a lock before assigning a new pointer to a CrossThreadPersistent node.
BUG=681488
Review-Url: https://codereview.chromium.org/2660463003
Cr-Commit-Position: refs/heads/master@{#449548}
Committed: https://chromium.googlesource.com/chromium/src/+/7c916610d4392731e6520d2bfd60c674303bb7a9
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix #Messages
Total messages: 23 (16 generated)
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix CrossThreadPersistent BUG= ========== to ========== Fix CrossThreadPersistent The root set in CrossThreadPersistent should not change while a thread is trying to mark as it may cause objects unmarked when it should be marked. This acquires a lock before assigning a new pointer to a CrossThreadPersistent node. BUG=681488 ==========
Description was changed from ========== Fix CrossThreadPersistent The root set in CrossThreadPersistent should not change while a thread is trying to mark as it may cause objects unmarked when it should be marked. This acquires a lock before assigning a new pointer to a CrossThreadPersistent node. BUG=681488 ========== to ========== CrossThreadPersistent should acquire a lock before assigning The root set in CrossThreadPersistent should not change while a thread is trying to mark as it may cause objects unmarked when it should be marked. This acquires a lock before assigning a new pointer to a CrossThreadPersistent node. BUG=681488 ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org, oilpan-reviews@chromium.org
I think I made a mistake. We already acquire the lock in PersistentBase::initialize/uninitialize but we don't when a PersistentNode already exists. If we have two CrossThreadPersistents A, B which point to objects a, b respectively. When the owner thread of objects a and b runs marking and it finishes tracing CrossThreadPersistent A and marks object a, the other thread could swap CrossThreadPersistents A and B, which would cause failure to mark object b.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
lgtm The CTP global lock could be a candidate for a cheaper, low-contention lock. https://codereview.chromium.org/2660463003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/Persistent.h (left): https://codereview.chromium.org/2660463003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/Persistent.h:178: releaseStore( With the lock held, this could be a 'normal' store.
Did you locate a particular assign() that ran into this subtle issue?
On 2017/02/09 07:46:49, sof wrote: > Did you locate a particular assign() that ran into this subtle issue? > Did you locate a particular assign() that ran into this subtle issue? No I didn't see an actual issue but I believe this will resolve this data race detected by Tsan https://bugs.chromium.org/p/chromium/issues/detail?id=681488. I observed something like this: 1. SQLTransactionBackend::m_currentStatementBackend is set to object A, owned by the main thread. 2. On the main thread, GC marking phase starts and acquires CrossThreadPersistentRegion lock. 3. On the db thread, object A is used. 4. On the db thread, SQLTransactionBackend::getNextStatement() begins to clear m_currentStatementBackend and PersistentBase::assign assigns nullptr to Persistentbase::m_raw 4. On the db thread, PersistentBase::uninitialize waits for CrossThreadPersistentRegion lock. 5. On the main thread, object A is destroyed in the sweep phase. I think this is flagged as a data race because object A is used in the db thread and destroyed in the main thread with no lock acquisition.
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/2660463003/#ps20001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486703948922190,
"parent_rev": "25b17c1a107cbc956487b5a6bb4698b986f6f035", "commit_rev":
"7c916610d4392731e6520d2bfd60c674303bb7a9"}
Message was sent while issue was closed.
Description was changed from ========== CrossThreadPersistent should acquire a lock before assigning The root set in CrossThreadPersistent should not change while a thread is trying to mark as it may cause objects unmarked when it should be marked. This acquires a lock before assigning a new pointer to a CrossThreadPersistent node. BUG=681488 ========== to ========== CrossThreadPersistent should acquire a lock before assigning The root set in CrossThreadPersistent should not change while a thread is trying to mark as it may cause objects unmarked when it should be marked. This acquires a lock before assigning a new pointer to a CrossThreadPersistent node. BUG=681488 Review-Url: https://codereview.chromium.org/2660463003 Cr-Commit-Position: refs/heads/master@{#449548} Committed: https://chromium.googlesource.com/chromium/src/+/7c916610d4392731e6520d2bfd60... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7c916610d4392731e6520d2bfd60... |
