Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2201373002:
Ignore invalid managed user settings values gracefully. (Closed)
DescriptionIgnore invalid managed user settings values gracefully.
If the managed user settings isn't a valid JSON value, don't store it.
Ignore it instead and log an error.
BUG=633961
Committed: https://crrev.com/d48f6178d1cfbf6b2dc26df2066343b778edb3d1
Cr-Commit-Position: refs/heads/master@{#409548}
Patch Set 1 #
Total comments: 6
Patch Set 2 : response to code review comments #
Total comments: 1
Patch Set 3 : response to more code review #Messages
Total messages: 14 (6 generated)
Description was changed from ========== Ignore invalid managed user settings values gracefully. If the managed user settings isn't a valid JSON value, don't store it. Ignore it instead and log an error. BUG=633961 ========== to ========== Ignore invalid managed user settings values gracefully. If the managed user settings isn't a valid JSON value, don't store it. Ignore it instead and log an error. BUG=633961 ==========
mamir@chromium.org changed reviewers: + bauerb@chromium.org, treib@chromium.org
https://codereview.chromium.org/2201373002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/2201373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_settings_service.cc:245: LOG(ERROR) << "Invalid managed user setting value:" DLOG please, so the message won't make it into release builds. https://codereview.chromium.org/2201373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_settings_service.cc:245: LOG(ERROR) << "Invalid managed user setting value:" nit: Space after : please
LGTM w/ Marc's comments addressed. https://codereview.chromium.org/2201373002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/2201373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_settings_service.cc:248: continue; Note that even if we compile out the log, we will still handle this in a Release build. Now, that is what we want (because it's preferable over crashing), but it might still be worth to add a comment here to explain that.
https://codereview.chromium.org/2201373002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/2201373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_settings_service.cc:245: LOG(ERROR) << "Invalid managed user setting value:" On 2016/08/03 14:40:32, Marc Treib wrote: > DLOG please, so the message won't make it into release builds. Done. https://codereview.chromium.org/2201373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_settings_service.cc:245: LOG(ERROR) << "Invalid managed user setting value:" On 2016/08/03 14:40:32, Marc Treib wrote: > nit: Space after : please Done. https://codereview.chromium.org/2201373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_settings_service.cc:248: continue; On 2016/08/03 14:44:07, Bernhard Bauer wrote: > Note that even if we compile out the log, we will still handle this in a Release > build. Now, that is what we want (because it's preferable over crashing), but it > might still be worth to add a comment here to explain that. Done.
LGTM with some comment nits :) https://codereview.chromium.org/2201373002/diff/20001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_settings_service.cc (right): https://codereview.chromium.org/2201373002/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_settings_service.cc:245: // dictionary. Therefore, we ignore them here to prevent such crashes. Just say that SetWithoutPathExpansion below requires non-null values. Also maybe mention *why* null values can occur here: For wrongly formatted input. Finally, nit: Start comment with a capital letter.
The CQ bit was checked by mamir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/2201373002/#ps40001 (title: "response to more code review")
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.
Description was changed from ========== Ignore invalid managed user settings values gracefully. If the managed user settings isn't a valid JSON value, don't store it. Ignore it instead and log an error. BUG=633961 ========== to ========== Ignore invalid managed user settings values gracefully. If the managed user settings isn't a valid JSON value, don't store it. Ignore it instead and log an error. BUG=633961 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ignore invalid managed user settings values gracefully. If the managed user settings isn't a valid JSON value, don't store it. Ignore it instead and log an error. BUG=633961 ========== to ========== Ignore invalid managed user settings values gracefully. If the managed user settings isn't a valid JSON value, don't store it. Ignore it instead and log an error. BUG=633961 Committed: https://crrev.com/d48f6178d1cfbf6b2dc26df2066343b778edb3d1 Cr-Commit-Position: refs/heads/master@{#409548} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d48f6178d1cfbf6b2dc26df2066343b778edb3d1 Cr-Commit-Position: refs/heads/master@{#409548} |
