Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(11)

Issue 2436403003: [DIAL] Make a copy of device id before erasing device from map. (Closed)

Created:
3 years, 1 month ago by imcheng
Modified:
3 years, 1 month ago
Reviewers:
mark a. foltz, miu, Wez
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[DIAL] Make a copy of device id before erasing device from map. erase() by key takes input param as a const ref. Invoking erase() by key using data that will be destroyed during the erase() call will result in undefined behavior. BUG=656073 Committed: https://crrev.com/2a375e615245ec9cc6ce32f755a5a46564a250e7 Cr-Commit-Position: refs/heads/master@{#426957}

Patch Set 1 #

Patch Set 2 : fmt #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -2 lines) Patch
M chrome/browser/extensions/api/dial/dial_registry.cc View 1 1 chunk +5 lines, -2 lines 2 comments Download

Messages

Total messages: 12 (5 generated)
imcheng
PTAL. Post-merge I do want to rewrite the loop so it doesn't use erase-by-key on ...
3 years, 1 month ago (2016-10-21 23:36:59 UTC) #2
imcheng
3 years, 1 month ago (2016-10-22 00:52:44 UTC) #4
Wez
lgtm https://codereview.chromium.org/2436403003/diff/20001/chrome/browser/extensions/api/dial/dial_registry.cc File chrome/browser/extensions/api/dial/dial_registry.cc (right): https://codereview.chromium.org/2436403003/diff/20001/chrome/browser/extensions/api/dial/dial_registry.cc#newcode167 chrome/browser/extensions/api/dial/dial_registry.cc:167: const size_t num_erased_by_id = device_by_id_map_.erase(device_id); As discussed offline... ...
3 years, 1 month ago (2016-10-22 00:55:39 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2436403003/20001
3 years, 1 month ago (2016-10-22 00:56:22 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (id:20001)
3 years, 1 month ago (2016-10-22 02:49:27 UTC) #8
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/2a375e615245ec9cc6ce32f755a5a46564a250e7 Cr-Commit-Position: refs/heads/master@{#426957}
3 years, 1 month ago (2016-10-22 02:52:00 UTC) #10
miu
3 years, 1 month ago (2016-10-22 18:37:19 UTC) #12
Message was sent while issue was closed.
https://codereview.chromium.org/2436403003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/dial_registry.cc (right):

https://codereview.chromium.org/2436403003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/dial_registry.cc:166: std::string device_id =
device->device_id();
nit: Probably not a big deal for performance, but the copy can be avoided by
using std::move() here. Though, IIRC, our std::strings are ref-counted, so
there's probably no copy going on internally.

Powered by Google App Engine
This is Rietveld 408576698