Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 7 months ago by achuithb Modified:
4 years, 6 months ago Reviewers:
haraken, jsbell, jianli CC:
chromium-reviews, darin-cc_chromium.org, jam, kinuko+fileapi, nhiroki, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix for sync filesystem app crash.
Previously removing the SafePointScope had a risk of producing a dead lock if the following scenario happens:
1) The main thread starts a GC and tries to stop all other threads.
2) The worker thread is waiting on a signal without entering the SafePointScope. The main thread cannot stop the worker thread forever and thus cannot make progress. Dead lock.
However, (to avoid this kind of dead lock) code has been added to timeout the GC if the GCing thread fails at stopping other threads within 100 ms. So not entering SafePointScope will just cause the timeout -- it won't cause a dead lock.
This fixes a hard-to-reproduce timing bug that's been a big concern of one of our partners.
BUG=592124, 608603
TEST=manual
Committed: https://crrev.com/f466a07e32ce1c8083dc726c7391e68b2ff2e7b0
Cr-Commit-Position: refs/heads/master@{#399265}
Patch Set 1 #
Created: 4 years, 7 months ago
Messages
Total messages: 22 (9 generated)
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009093002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2009093002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
achuith@chromium.org changed reviewers: + jsbell@chromium.org
PTAL Joshua
Per our offline discussion, LGTM (though I'm not an owner). Please add the rationale to the CL description.
Description was changed from ========== Fix for sync filesystem app crash. BUG=592124 ========== to ========== Fix for sync filesystem app crash. Previously removing the SafePointScope had a risk of producing a dead lock if the following scenario happens: 1) The main thread starts a GC and tries to stop all other threads. 2) The worker thread is waiting on a signal without entering the SafePointScope. The main thread cannot stop the worker thread forever and thus cannot make progress. Dead lock. However, (to avoid this kind of dead lock) code has been added to timeout the GC if the GCing thread fails at stopping other threads within 100 ms. So not entering SafePointScope will just cause the timeout -- it won't cause a dead lock. This fixes a hard-to-reproduce timing bug that's been a big concern of one of our partners. BUG=592124 TEST=manual ==========
On 2016/06/09 23:46:19, haraken wrote: > Per our offline discussion, LGTM (though I'm not an owner). > > Please add the rationale to the CL description. Done
achuith@chromium.org changed reviewers: + jianli@chromium.org
Jian Li: Could I please get an owner lgtm?
lgtm
lgtm
Description was changed from ========== Fix for sync filesystem app crash. Previously removing the SafePointScope had a risk of producing a dead lock if the following scenario happens: 1) The main thread starts a GC and tries to stop all other threads. 2) The worker thread is waiting on a signal without entering the SafePointScope. The main thread cannot stop the worker thread forever and thus cannot make progress. Dead lock. However, (to avoid this kind of dead lock) code has been added to timeout the GC if the GCing thread fails at stopping other threads within 100 ms. So not entering SafePointScope will just cause the timeout -- it won't cause a dead lock. This fixes a hard-to-reproduce timing bug that's been a big concern of one of our partners. BUG=592124 TEST=manual ========== to ========== Fix for sync filesystem app crash. Previously removing the SafePointScope had a risk of producing a dead lock if the following scenario happens: 1) The main thread starts a GC and tries to stop all other threads. 2) The worker thread is waiting on a signal without entering the SafePointScope. The main thread cannot stop the worker thread forever and thus cannot make progress. Dead lock. However, (to avoid this kind of dead lock) code has been added to timeout the GC if the GCing thread fails at stopping other threads within 100 ms. So not entering SafePointScope will just cause the timeout -- it won't cause a dead lock. This fixes a hard-to-reproduce timing bug that's been a big concern of one of our partners. BUG=592124,608603 TEST=manual ==========
The CQ bit was checked by achuith@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009093002/1
Message was sent while issue was closed.
Description was changed from ========== Fix for sync filesystem app crash. Previously removing the SafePointScope had a risk of producing a dead lock if the following scenario happens: 1) The main thread starts a GC and tries to stop all other threads. 2) The worker thread is waiting on a signal without entering the SafePointScope. The main thread cannot stop the worker thread forever and thus cannot make progress. Dead lock. However, (to avoid this kind of dead lock) code has been added to timeout the GC if the GCing thread fails at stopping other threads within 100 ms. So not entering SafePointScope will just cause the timeout -- it won't cause a dead lock. This fixes a hard-to-reproduce timing bug that's been a big concern of one of our partners. BUG=592124,608603 TEST=manual ========== to ========== Fix for sync filesystem app crash. Previously removing the SafePointScope had a risk of producing a dead lock if the following scenario happens: 1) The main thread starts a GC and tries to stop all other threads. 2) The worker thread is waiting on a signal without entering the SafePointScope. The main thread cannot stop the worker thread forever and thus cannot make progress. Dead lock. However, (to avoid this kind of dead lock) code has been added to timeout the GC if the GCing thread fails at stopping other threads within 100 ms. So not entering SafePointScope will just cause the timeout -- it won't cause a dead lock. This fixes a hard-to-reproduce timing bug that's been a big concern of one of our partners. BUG=592124,608603 TEST=manual ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Fix for sync filesystem app crash. Previously removing the SafePointScope had a risk of producing a dead lock if the following scenario happens: 1) The main thread starts a GC and tries to stop all other threads. 2) The worker thread is waiting on a signal without entering the SafePointScope. The main thread cannot stop the worker thread forever and thus cannot make progress. Dead lock. However, (to avoid this kind of dead lock) code has been added to timeout the GC if the GCing thread fails at stopping other threads within 100 ms. So not entering SafePointScope will just cause the timeout -- it won't cause a dead lock. This fixes a hard-to-reproduce timing bug that's been a big concern of one of our partners. BUG=592124,608603 TEST=manual ========== to ========== Fix for sync filesystem app crash. Previously removing the SafePointScope had a risk of producing a dead lock if the following scenario happens: 1) The main thread starts a GC and tries to stop all other threads. 2) The worker thread is waiting on a signal without entering the SafePointScope. The main thread cannot stop the worker thread forever and thus cannot make progress. Dead lock. However, (to avoid this kind of dead lock) code has been added to timeout the GC if the GCing thread fails at stopping other threads within 100 ms. So not entering SafePointScope will just cause the timeout -- it won't cause a dead lock. This fixes a hard-to-reproduce timing bug that's been a big concern of one of our partners. BUG=592124,608603 TEST=manual Committed: https://crrev.com/f466a07e32ce1c8083dc726c7391e68b2ff2e7b0 Cr-Commit-Position: refs/heads/master@{#399265} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/f466a07e32ce1c8083dc726c7391e68b2ff2e7b0 Cr-Commit-Position: refs/heads/master@{#399265} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
