Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846593005:
Relanding #2: Fixing crash that can occur if signal is modified while firing. (Closed)
|
Created:
3 years, 8 months ago by Taylor Brandstetter Modified:
3 years, 7 months ago Reviewers:
pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRelanding #2: Fixing crash that can occur if signal is modified while firing.
The crash occurs if a slot causes the very next slot in iteration order
to be disconnected.
Relanding after fixing a race condition that this CL revealed. Previously
the race resulted in an invalidated iterator, but now it will result in the
iterator being modified, so TSan catches it.
BUG=webrtc:7527
Review-Url: https://codereview.webrtc.org/2846593005
Cr-Commit-Position: refs/heads/master@{#18124}
Committed: https://chromium.googlesource.com/external/webrtc/+/4483af35e91e811042b5fd13a1b94aed8c835930
Patch Set 1 #Patch Set 2 : Fixing silly mistakes #Patch Set 3 : Merge with master, and improve test slightly. #Patch Set 4 : Removing a signal that was being accessed unsafely from two threads, and wasn't needed in the first… #Patch Set 5 : Fixing issue with disconnect_all, adding test, simplifying code a bit #
Messages
Total messages: 38 (24 generated)
deadbeef@webrtc.org changed reviewers: + pthatcher@webrtc.org
PTAL. Another sigslot issue Noah found for us.
lgtm
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2846593005/#ps40001 (title: "Merge with master, and improve test slightly.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_msan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/20057)
The CQ bit was checked by deadbeef@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493454712331930,
"parent_rev": "8b1d862bfe6b8c160b0130e7d0b0444bcf668dd4", "commit_rev":
"961c2adf1ea25bcc40ad64cae1ac2ea258e26de2"}
Message was sent while issue was closed.
Description was changed from ========== Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. BUG=webrtc:7527 ========== to ========== Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. BUG=webrtc:7527 Review-Url: https://codereview.webrtc.org/2846593005 Cr-Commit-Position: refs/heads/master@{#17943} Committed: https://chromium.googlesource.com/external/webrtc/+/961c2adf1ea25bcc40ad64cae... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/961c2adf1ea25bcc40ad64cae...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/2847243006/ by deadbeef@webrtc.org. The reason for reverting is: Revealed a race condition (now reported by TSan), and breaks the scenario where a signal is deleted while firing..
Message was sent while issue was closed.
Description was changed from ========== Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. BUG=webrtc:7527 Review-Url: https://codereview.webrtc.org/2846593005 Cr-Commit-Position: refs/heads/master@{#17943} Committed: https://chromium.googlesource.com/external/webrtc/+/961c2adf1ea25bcc40ad64cae... ========== to ========== Relanding: Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. Relanding after fixing a race condition that this CL revealed. Previously the race resulted in an invalidated iterator, but now it will result in the iterator being modified, so TSan catches it. BUG=webrtc:7527 Review-Url: https://codereview.webrtc.org/2846593005 Cr-Commit-Position: refs/heads/master@{#17943} Committed: https://chromium.googlesource.com/external/webrtc/+/961c2adf1ea25bcc40ad64cae... ==========
The CQ bit was checked by deadbeef@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you take another look? This revealed a TSan race for a signal that ended up not being needed in the first place (see CL description).
lgtm
The CQ bit was checked by deadbeef@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493701014931640,
"parent_rev": "0153ca5a612126e71fe888416a10cd6e68bce22e", "commit_rev":
"fc1af0155718d029a97fddfd64422e9e6e509809"}
Message was sent while issue was closed.
Description was changed from ========== Relanding: Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. Relanding after fixing a race condition that this CL revealed. Previously the race resulted in an invalidated iterator, but now it will result in the iterator being modified, so TSan catches it. BUG=webrtc:7527 Review-Url: https://codereview.webrtc.org/2846593005 Cr-Commit-Position: refs/heads/master@{#17943} Committed: https://chromium.googlesource.com/external/webrtc/+/961c2adf1ea25bcc40ad64cae... ========== to ========== Relanding: Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. Relanding after fixing a race condition that this CL revealed. Previously the race resulted in an invalidated iterator, but now it will result in the iterator being modified, so TSan catches it. BUG=webrtc:7527 Review-Url: https://codereview.webrtc.org/2846593005 Cr-Original-Commit-Position: refs/heads/master@{#17943} Committed: https://chromium.googlesource.com/external/webrtc/+/961c2adf1ea25bcc40ad64cae... Review-Url: https://codereview.webrtc.org/2846593005 Cr-Commit-Position: refs/heads/master@{#17965} Committed: https://chromium.googlesource.com/external/webrtc/+/fc1af0155718d029a97fddfd6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/fc1af0155718d029a97fddfd6...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.webrtc.org/2859443002/ by perkj@webrtc.org. The reason for reverting is: Breaks XmppConnectionTest in Chrome. https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....
Message was sent while issue was closed.
Description was changed from ========== Relanding: Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. Relanding after fixing a race condition that this CL revealed. Previously the race resulted in an invalidated iterator, but now it will result in the iterator being modified, so TSan catches it. BUG=webrtc:7527 Review-Url: https://codereview.webrtc.org/2846593005 Cr-Original-Commit-Position: refs/heads/master@{#17943} Committed: https://chromium.googlesource.com/external/webrtc/+/961c2adf1ea25bcc40ad64cae... Review-Url: https://codereview.webrtc.org/2846593005 Cr-Commit-Position: refs/heads/master@{#17965} Committed: https://chromium.googlesource.com/external/webrtc/+/fc1af0155718d029a97fddfd6... ========== to ========== Relanding #2: Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. Relanding after fixing a race condition that this CL revealed. Previously the race resulted in an invalidated iterator, but now it will result in the iterator being modified, so TSan catches it. BUG=webrtc:7527 ==========
The CQ bit was checked by deadbeef@webrtc.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.webrtc.org/...
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 deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2846593005/#ps80001 (title: "Fixing issue with disconnect_all, adding test, simplifying code a bit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494603717924330,
"parent_rev": "581d5ce00b1d5e27d8987e698c9be7ba9627cff5", "commit_rev":
"4483af35e91e811042b5fd13a1b94aed8c835930"}
Message was sent while issue was closed.
Description was changed from ========== Relanding #2: Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. Relanding after fixing a race condition that this CL revealed. Previously the race resulted in an invalidated iterator, but now it will result in the iterator being modified, so TSan catches it. BUG=webrtc:7527 ========== to ========== Relanding #2: Fixing crash that can occur if signal is modified while firing. The crash occurs if a slot causes the very next slot in iteration order to be disconnected. Relanding after fixing a race condition that this CL revealed. Previously the race resulted in an invalidated iterator, but now it will result in the iterator being modified, so TSan catches it. BUG=webrtc:7527 Review-Url: https://codereview.webrtc.org/2846593005 Cr-Commit-Position: refs/heads/master@{#18124} Committed: https://chromium.googlesource.com/external/webrtc/+/4483af35e91e811042b5fd13a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/4483af35e91e811042b5fd13a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
