|
|
Created:
4 years, 6 months ago by stefanocs Modified:
4 years, 5 months ago CC:
chromium-reviews, grt+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@add-hooks-to-permission-layer Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd throttling to permission reporter
Report sending is restricted to a maximum of 5 reports per minute per origin per permission.
BUG=613883
Committed: https://crrev.com/3c0cfd954534699d67193f0d96281c3fb046c8f9
Cr-Commit-Position: refs/heads/master@{#406236}
Patch Set 1 #Patch Set 2 : Use queue instead of set to store sent time #Patch Set 3 : Use existing hash combine function #Patch Set 4 : Rename constant #
Total comments: 6
Patch Set 5 : Resolve nits #Patch Set 6 : Add throttling test #Patch Set 7 : Remove a constructor #Patch Set 8 : Add comment #
Total comments: 12
Patch Set 9 : Resolve comments and rebase #
Total comments: 6
Patch Set 10 : Resolve comments #
Total comments: 6
Patch Set 11 : Resolve comments #
Total comments: 10
Patch Set 12 : Rename member variable sent_histories #Patch Set 13 : todo #Patch Set 14 : todo #Patch Set 15 : typo #
Depends on Patchset: Messages
Total messages: 42 (16 generated)
stefanocs@google.com changed reviewers: + kcarattini@chromium.org, raymes@chromium.org
We should also add a test for this. What you can do is pass in a base::Clock. Then you can pass in a DefaultClock in production code and a SimpleTestClock in test code, which allows you to set times. https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:25: const int kMaximumReportsPerOriginPerPermission = 5; ...PerMinute? https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:75: // otherwise false. nit: probably don't reference the constant here, since it's not defined in this file - just say "is under a threshold" or something similar https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:81: std::queue<base::Time>, I think perhaps we can simplify this a little bit. What if we store a timestamp whenever a send occurs and then just count the number of sends that happen since that timestamp. If the current time is more than 1 min after the timestamp we can reset everything. Then we could just store: base::Time send_timestamp_; std::unordered_map<PermissionAndOrigin, int> send_counts_;
https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:25: const int kMaximumReportsPerOriginPerPermission = 5; On 2016/06/27 06:26:56, raymes wrote: > ...PerMinute? Done. https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:75: // otherwise false. On 2016/06/27 06:26:56, raymes wrote: > nit: probably don't reference the constant here, since it's not defined in this > file - just say "is under a threshold" or something similar Done. https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:81: std::queue<base::Time>, On 2016/06/27 06:26:56, raymes wrote: > I think perhaps we can simplify this a little bit. What if we store a timestamp > whenever a send occurs and then just count the number of sends that happen since > that timestamp. If the current time is more than 1 min after the timestamp we > can reset everything. > > Then we could just store: > > base::Time send_timestamp_; > std::unordered_map<PermissionAndOrigin, int> send_counts_; > I think it would be possible to have more than five reports in window of one minute like if reports are sent at 0, 57, 58, 59, 60, (reset here), 61, 62, 63, 64, 65 (in sec). While it still does prevents spamming to the server, it does not exactly follow the definition, is that ok?
Test added.
https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:143: current_time - history.front() > base::TimeDelta::FromMinutes(1)) nit: add {} https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:37: }; nit: If we define these as nested classes within PermissionReporter, we can just forward declare them in the header and have the complexity defined in the cc file which might be a bit nicer. https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:87: sent_histories; It seems like we might need this complexity then unless we change the definition. Let's get Kendra's thoughts. https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:32: const PermissionReport::PermissionType kDummyPermissionReportPermissionOne = nit: maybe we just inline this? Sometimes it's a bit easier to read than having a bunch of constants all defined at the top. https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:34: const PermissionReport::Action kDummyPermissionReportAction = nit: same here. https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:120: EXPECT_TRUE(IsAllowedToSend(kDummyPermissionOne, GURL(kDummyOriginOne))); Instead of calling IsAllowedToSend directly, I think it would be better to test the public interface by just sending reports. We can use the MockReportSender to check if the reports actually got sent (we don't need to check their contents though).
https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:143: current_time - history.front() > base::TimeDelta::FromMinutes(1)) On 2016/06/29 07:09:17, raymes wrote: > nit: add {} Done. https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:37: }; On 2016/06/29 07:09:17, raymes wrote: > nit: If we define these as nested classes within PermissionReporter, we can just > forward declare them in the header and have the complexity defined in the cc > file which might be a bit nicer. Do you mean these structs should be forward declared in this header file and have the declaration and definition in the cc file? That's what I was trying to do but the hash table needs to know the declaration of these structs in the this file. https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:87: sent_histories; On 2016/06/29 07:09:17, raymes wrote: > It seems like we might need this complexity then unless we change the > definition. Let's get Kendra's thoughts. Acknowledged. https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:32: const PermissionReport::PermissionType kDummyPermissionReportPermissionOne = On 2016/06/29 07:09:17, raymes wrote: > nit: maybe we just inline this? Sometimes it's a bit easier to read than having > a bunch of constants all defined at the top. Done. https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:34: const PermissionReport::Action kDummyPermissionReportAction = On 2016/06/29 07:09:17, raymes wrote: > nit: same here. Done. https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:120: EXPECT_TRUE(IsAllowedToSend(kDummyPermissionOne, GURL(kDummyOriginOne))); On 2016/06/29 07:09:17, raymes wrote: > Instead of calling IsAllowedToSend directly, I think it would be better to test > the public interface by just sending reports. We can use the MockReportSender to > check if the reports actually got sent (we don't need to check their contents > though). Done.
lgtm with a couple of comments We should still see if we want to simplify the code by altering the definition slightly. https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:71: int NumberOfNewReports() { nit: GetAndResetNumberReports https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:92: permission_reporter_( We should probably create/reset this in a SetUp method to ensure the state gets reset for every test. https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:215: EXPECT_EQ(1, mock_report_sender_->NumberOfNewReports()); Maybe have one more test where we add the reports at different times over a minute and test that only some of them get evicted when advancing time?
https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:71: int NumberOfNewReports() { On 2016/06/30 01:30:19, raymes wrote: > nit: GetAndResetNumberReports Done. https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:92: permission_reporter_( On 2016/06/30 01:30:19, raymes wrote: > We should probably create/reset this in a SetUp method to ensure the state gets > reset for every test. Done. https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:215: EXPECT_EQ(1, mock_report_sender_->NumberOfNewReports()); On 2016/06/30 01:30:19, raymes wrote: > Maybe have one more test where we add the reports at different times over a > minute and test that only some of them get evicted when advancing time? Done.
https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:65: explicit PermissionReporter(std::unique_ptr<net::ReportSender> report_sender, This no longer needs to be explicit with 2 arguments. https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:80: bool IsAllowedToSend(content::PermissionType permission, const GURL& origin); How about something a bit more descriptive to what this method is doing? Maybe "ReportThresholdExceeded" or something similar (although you'd need to swap the return values with that name)? https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:71: int GetAndResetNumberReports() { How about "GetAndResetNumberOfReportsSent"?
https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:65: explicit PermissionReporter(std::unique_ptr<net::ReportSender> report_sender, On 2016/07/06 06:28:20, kcarattini wrote: > This no longer needs to be explicit with 2 arguments. Done. https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:80: bool IsAllowedToSend(content::PermissionType permission, const GURL& origin); On 2016/07/06 06:28:20, kcarattini wrote: > How about something a bit more descriptive to what this method is doing? Maybe > "ReportThresholdExceeded" or something similar (although you'd need to swap the > return values with that name)? Done. https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:71: int GetAndResetNumberReports() { On 2016/07/06 06:28:20, kcarattini wrote: > How about "GetAndResetNumberOfReportsSent"? Done.
lgtm
stefanocs@google.com changed reviewers: + nparker@chromium.org
stefanocs@google.com changed reviewers: + nparker@chromium.org
nparker@chromium.org: Please review changes in chrome/browser/safe_browsing/ Thanks
nparker@chromium.org: Please review changes in chrome/browser/safe_browsing/ Thanks
ping :)
ping Nathan :)
lgtm Sorry for the delay! CLs were getting slurped up by Inbox. https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:158: std::queue<base::Time>& history = sent_histories[{permission, origin}]; A caveat, probably with no action required: This map will collect entries for every permission granted, and will never delete them when they're no longer useful. I think this is probably fine, since the rate of new permission requests x origins shouldn't be huge. If that is a problem you could store [ {origin, permission, timestamp}, {.., .., ..}, ... ] And then prune that list of old timestamps every time you hit this function. Then you'd need to count those that match origin + permission. https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:36: const PermissionAndOrigin& permission_and_origin) const; Rather than defining this in the .h file, can you have it use the default hasher, and then define std::hash<PermissionAndOrigin> in the .cc file? That'd avoid extra stuff in the .h file. If that doesn't work, then you can leave it here. https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:88: sent_histories; Should end in underscore. How about report_log_? or report_history_. Or what you have, I'm not picky.
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:158: std::queue<base::Time>& history = sent_histories[{permission, origin}]; On 2016/07/14 22:00:24, Nathan Parker wrote: > A caveat, probably with no action required: > > This map will collect entries for every permission granted, and will never > delete them when they're no longer useful. I think this is probably fine, since > the rate of new permission requests x origins shouldn't be huge. > > If that is a problem you could store > [ {origin, permission, timestamp}, {.., .., ..}, ... ] > > And then prune that list of old timestamps every time you hit this function. > Then you'd need to count those that match origin + permission. > Yes, me and Raymes have discussed this before and we think this is okay. Kendra, what do you think? https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:36: const PermissionAndOrigin& permission_and_origin) const; On 2016/07/14 22:00:24, Nathan Parker wrote: > Rather than defining this in the .h file, can you have it use the default > hasher, and then define std::hash<PermissionAndOrigin> in the .cc file? That'd > avoid extra stuff in the .h file. If that doesn't work, then you can leave it > here. This doesn't seem to work. I added this: namespace std { template <> struct hash<safe_browsing::PermissionAndOrigin> { public: std::size_t operator()(const safe_browsing::PermissionAndOrigin& permission_and_origin) const { std::size_t permission_hash = static_cast<std::size_t>(permission_and_origin.permission); std::size_t origin_hash = std::hash<std::string>()(permission_and_origin.origin.spec()); return base::HashInts(permission_hash, origin_hash); } }; But I was getting CE. https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:88: sent_histories; On 2016/07/14 22:00:24, Nathan Parker wrote: > Should end in underscore. > How about report_log_? or report_history_. Or what you have, I'm not picky. Done.
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:36: const PermissionAndOrigin& permission_and_origin) const; On 2016/07/15 02:04:29, stefanocs wrote: > On 2016/07/14 22:00:24, Nathan Parker wrote: > > Rather than defining this in the .h file, can you have it use the default > > hasher, and then define std::hash<PermissionAndOrigin> in the .cc file? > That'd > > avoid extra stuff in the .h file. If that doesn't work, then you can leave it > > here. > > This doesn't seem to work. > > I added this: > > namespace std { > template <> > struct hash<safe_browsing::PermissionAndOrigin> { > public: > std::size_t operator()(const safe_browsing::PermissionAndOrigin& > permission_and_origin) const { > std::size_t permission_hash = > static_cast<std::size_t>(permission_and_origin.permission); > std::size_t origin_hash = > std::hash<std::string>()(permission_and_origin.origin.spec()); > return base::HashInts(permission_hash, origin_hash); > } > }; > > But I was getting CE. I was thinking std::size_t std::hash<safe_browsing::PermissionAndOrigin>(safe_browsing::PermissionAndOrigin o) { .. } (not sure if the <..> is required). Then no struct is required. Maybe that doesn't work.
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:158: std::queue<base::Time>& history = sent_histories[{permission, origin}]; We also discussed just keeping: {(origin, permissions, timestamp): count} If timestamp is older than 1min we reset count. This gets an approximation of the limit but is much simpler to implement, requires less storage, etc.
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:158: std::queue<base::Time>& history = sent_histories[{permission, origin}]; On 2016/07/18 01:16:51, raymes wrote: > We also discussed just keeping: > {(origin, permissions, timestamp): count} > > If timestamp is older than 1min we reset count. This gets an approximation of > the limit but is much simpler to implement, requires less storage, etc. In the normal case I wouldn't expect this to be a problem. How about a TODO to address this if memory becomes an issue?
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:36: const PermissionAndOrigin& permission_and_origin) const; On 2016/07/15 23:39:40, Nathan Parker wrote: > On 2016/07/15 02:04:29, stefanocs wrote: > > On 2016/07/14 22:00:24, Nathan Parker wrote: > > > Rather than defining this in the .h file, can you have it use the default > > > hasher, and then define std::hash<PermissionAndOrigin> in the .cc file? > > That'd > > > avoid extra stuff in the .h file. If that doesn't work, then you can leave > it > > > here. > > > > This doesn't seem to work. > > > > I added this: > > > > namespace std { > > template <> > > struct hash<safe_browsing::PermissionAndOrigin> { > > public: > > std::size_t operator()(const safe_browsing::PermissionAndOrigin& > > permission_and_origin) const { > > std::size_t permission_hash = > > static_cast<std::size_t>(permission_and_origin.permission); > > std::size_t origin_hash = > > std::hash<std::string>()(permission_and_origin.origin.spec()); > > return base::HashInts(permission_hash, origin_hash); > > } > > }; > > > > But I was getting CE. > > > I was thinking > > std::size_t > std::hash<safe_browsing::PermissionAndOrigin>(safe_browsing::PermissionAndOrigin > o) { > .. > } > > (not sure if the <..> is required). Then no struct is required. Maybe that > doesn't work. Hmmm, that doesn't seem to work. It complains about "template specialization requires 'template<>'" and then "no function template matches function template specialization 'hash'".
lgtm Yea I may be smoking something on the hash<>() thing. Do what works.
The CQ bit was checked by stefanocs@google.com 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 stefanocs@google.com 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 checked by stefanocs@google.com 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 stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, kcarattini@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2089383005/#ps270001 (title: "typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add throttling to permission reporter Report sending is restricted to a maximum of 5 reports per minute per origin per permission. BUG=613883 ========== to ========== Add throttling to permission reporter Report sending is restricted to a maximum of 5 reports per minute per origin per permission. BUG=613883 Committed: https://crrev.com/3c0cfd954534699d67193f0d96281c3fb046c8f9 Cr-Commit-Position: refs/heads/master@{#406236} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/3c0cfd954534699d67193f0d96281c3fb046c8f9 Cr-Commit-Position: refs/heads/master@{#406236} |