Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784323002:
MD Settings: Second try at fixing ResetSettingsHandler crash (Closed)
|
Created:
3 years, 8 months ago by tommycli Modified:
3 years, 8 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, scottchen, Dan Beam Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Second try at fixing ResetSettingsHandler crash
ResetSettingsHandler::ResetProfile is run as a callback owned by
the |config_fetcher_| member.
However, |config_fetcher_| is also deleted by the ResetProfile method.
Since ResetProfile takes a const-ref parameter that's owned by
|config_fetcher_|, it clobbers its own parameter.
This CL fixes that issue by moving the callback to the stack before
the object is deleted from the heap.
BUG=660847
Review-Url: https://codereview.chromium.org/2784323002
Cr-Commit-Position: refs/heads/master@{#462122}
Committed: https://chromium.googlesource.com/chromium/src/+/890cfea7950ebc5910372deb91ebcbf665362817
Patch Set 1 #
Total comments: 3
Patch Set 2 : fix up here #Patch Set 3 : fix #
Total comments: 3
Messages
Total messages: 43 (25 generated)
tommycli@chromium.org changed reviewers: + dschuyler@chromium.org
dschuyler: PTAL, thanks! https://codereview.chromium.org/2784323002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right): https://codereview.chromium.org/2784323002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/reset_settings_handler.cc:148: config_fetcher_->SetCallback(base::Bind(&ResetSettingsHandler::ResetProfile, This just reverts the previous unsuccessful fix.
The CQ bit was checked by tommycli@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2784323002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right): https://codereview.chromium.org/2784323002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/reset_settings_handler.cc:253: weak_ptr_factory_.GetWeakPtr(), callback_id, send_settings, I'm missing how this is really different, since afaik a const reference is copied by bind. See https://cs.chromium.org/chromium/src/docs/callback.md?type=cs&q=/docs/callbac... My concern is that there is still something that needs fixing (if this is really copying either way, then while this is unlikely to hurt, it wouldn't be a fix).
https://codereview.chromium.org/2784323002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right): https://codereview.chromium.org/2784323002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/reset_settings_handler.cc:253: weak_ptr_factory_.GetWeakPtr(), callback_id, send_settings, On 2017/03/30 22:32:07, dschuyler wrote: > I'm missing how this is really different, since afaik a const reference is > copied by bind. > See > https://cs.chromium.org/chromium/src/docs/callback.md?type=cs&q=/docs/callbac... > > My concern is that there is still something that needs fixing (if this is really > copying either way, then while this is unlikely to hurt, it wouldn't be a fix). Just an update fyi note: Tommy and I talked in person and it's the config_fetcher_.reset(); on line 240 that needs to be considered here - the owner of the callback data may be reset while inside the callback.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
i'm not really sure what's going on here but it seems more legit than that last time we tried to fix this crash, IMO
Description was changed from ========== MD Settings: Second try at fixing ResetSettingsHandler crash ResetSettingsHandler::ResetProfile is run as a callback owned by the |config_fetcher_| member. However, |config_fetcher_| is also deleted by the ResetProfile method. Since ResetProfile takes a const-ref parameter that's owned by |config_fetcher_|, it clobbers its own parameter. This CL fixes that by passing the parameter by value. BUG=660847 ========== to ========== MD Settings: Second try at fixing ResetSettingsHandler crash ResetSettingsHandler::ResetProfile is run as a callback owned by the |config_fetcher_| member. However, |config_fetcher_| is also deleted by the ResetProfile method. Since ResetProfile takes a const-ref parameter that's owned by |config_fetcher_|, it clobbers its own parameter. This CL fixes that by passing the parameter by value. BUG=660847 ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
Description was changed from ========== MD Settings: Second try at fixing ResetSettingsHandler crash ResetSettingsHandler::ResetProfile is run as a callback owned by the |config_fetcher_| member. However, |config_fetcher_| is also deleted by the ResetProfile method. Since ResetProfile takes a const-ref parameter that's owned by |config_fetcher_|, it clobbers its own parameter. This CL fixes that by passing the parameter by value. BUG=660847 ========== to ========== MD Settings: Second try at fixing ResetSettingsHandler crash ResetSettingsHandler::ResetProfile is run as a callback owned by the |config_fetcher_| member. However, |config_fetcher_| is also deleted by the ResetProfile method. Since ResetProfile takes a const-ref parameter that's owned by |config_fetcher_|, it clobbers its own parameter. This CL fixes that issue by moving the callback to the stack before deletion. BUG=660847 ==========
Description was changed from ========== MD Settings: Second try at fixing ResetSettingsHandler crash ResetSettingsHandler::ResetProfile is run as a callback owned by the |config_fetcher_| member. However, |config_fetcher_| is also deleted by the ResetProfile method. Since ResetProfile takes a const-ref parameter that's owned by |config_fetcher_|, it clobbers its own parameter. This CL fixes that issue by moving the callback to the stack before deletion. BUG=660847 ========== to ========== MD Settings: Second try at fixing ResetSettingsHandler crash ResetSettingsHandler::ResetProfile is run as a callback owned by the |config_fetcher_| member. However, |config_fetcher_| is also deleted by the ResetProfile method. Since ResetProfile takes a const-ref parameter that's owned by |config_fetcher_|, it clobbers its own parameter. This CL fixes that issue by moving the callback to the stack before the object is deleted from the heap. BUG=660847 ==========
The CQ bit was checked by tommycli@chromium.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.chromium.or...
PTAL, hey guys, I dug into this some more this morning, and I'm sure now that the latest patchset contains the RIGHT FIX. See https://cs.chromium.org/chromium/src/docs/callback.md?type=cs&q=ResetAndRetur... for documentation on this topic. It's the first time I've come across this scenario, but it exists in many places in the Chrome codebase.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I like the direction this is going :) ResetAndReturn() is handy to know about. https://codereview.chromium.org/2784323002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right): https://codereview.chromium.org/2784323002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/reset_settings_handler.cc:239: default_settings = config_fetcher_->GetSettings(); Can |config_fetcher_| be true (at line 237) with the new calls to base::ResetAndReturn()? If it will now always be false, are we losing the default_settings from the |config_fetcher_| here?
The CQ bit was checked by tommycli@chromium.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.chromium.or...
comment below. thanks! https://codereview.chromium.org/2784323002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right): https://codereview.chromium.org/2784323002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/reset_settings_handler.cc:239: default_settings = config_fetcher_->GetSettings(); On 2017/03/31 19:21:39, dschuyler wrote: > Can |config_fetcher_| be true (at line 237) with the new calls to > base::ResetAndReturn()? If it will now always be false, are we losing the > default_settings from the |config_fetcher_| here? Hi, yes, that will remain true. The base::ResetAndReturn call is within config_fetcher_ and resets a member of config_fetcher_, not config_fetcher_ itself.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
because most of our CLs have been speculative while watching for new crashes, I'm happy to land this to try (and tommycli@ sounds more much more confident now) lgtm
lgtm https://codereview.chromium.org/2784323002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right): https://codereview.chromium.org/2784323002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/reset_settings_handler.cc:239: default_settings = config_fetcher_->GetSettings(); On 2017/04/03 15:34:23, tommycli wrote: > On 2017/03/31 19:21:39, dschuyler wrote: > > Can |config_fetcher_| be true (at line 237) with the new calls to > > base::ResetAndReturn()? If it will now always be false, are we losing the > > default_settings from the |config_fetcher_| here? > > Hi, yes, that will remain true. The base::ResetAndReturn call is within > config_fetcher_ and resets a member of config_fetcher_, not config_fetcher_ > itself. Cool. Yeah, that makes sense.
On 2017/04/03 18:27:10, dschuyler wrote: > lgtm > > https://codereview.chromium.org/2784323002/diff/40001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/settings/reset_settings_handler.cc (right): > > https://codereview.chromium.org/2784323002/diff/40001/chrome/browser/ui/webui... > chrome/browser/ui/webui/settings/reset_settings_handler.cc:239: default_settings > = config_fetcher_->GetSettings(); > On 2017/04/03 15:34:23, tommycli wrote: > > On 2017/03/31 19:21:39, dschuyler wrote: > > > Can |config_fetcher_| be true (at line 237) with the new calls to > > > base::ResetAndReturn()? If it will now always be false, are we losing the > > > default_settings from the |config_fetcher_| here? > > > > Hi, yes, that will remain true. The base::ResetAndReturn call is within > > config_fetcher_ and resets a member of config_fetcher_, not config_fetcher_ > > itself. > > Cool. Yeah, that makes sense. Awesome... sending it in.
The CQ bit was checked by tommycli@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tommycli@chromium.org changed reviewers: + battre@chromium.org
battre: PTAL, thanks!
On 2017/04/03 19:01:55, tommycli wrote: > battre: PTAL, thanks! battre: ping, thanks!
LGTM I wonder though, whether a simpler/cleaner way would be to asynchronously call the callback via PostTask.
On 2017/04/05 12:31:36, battre wrote: > LGTM > > I wonder though, whether a simpler/cleaner way would be to asynchronously call > the callback via PostTask. Hey battre: PostTask was actually my first attempt, but I discarded that because: 1) Causes an extra unnecessary bounce to a message loop. 2) I found myself writing a comment explaining why we used PostTask (to give PostTask ownership of the callback). base::ResetAndReturn was more explicit because its only purpose is to allow for callbacks that destroy the callback-owning object. base::ResetAndReturn is also used all over the code, so it's a pretty common idiom. Thanks!
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491408453770790,
"parent_rev": "ebf6e4d5323c4bcb0b8c3e048deff33e94bbd971", "commit_rev":
"890cfea7950ebc5910372deb91ebcbf665362817"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Second try at fixing ResetSettingsHandler crash ResetSettingsHandler::ResetProfile is run as a callback owned by the |config_fetcher_| member. However, |config_fetcher_| is also deleted by the ResetProfile method. Since ResetProfile takes a const-ref parameter that's owned by |config_fetcher_|, it clobbers its own parameter. This CL fixes that issue by moving the callback to the stack before the object is deleted from the heap. BUG=660847 ========== to ========== MD Settings: Second try at fixing ResetSettingsHandler crash ResetSettingsHandler::ResetProfile is run as a callback owned by the |config_fetcher_| member. However, |config_fetcher_| is also deleted by the ResetProfile method. Since ResetProfile takes a const-ref parameter that's owned by |config_fetcher_|, it clobbers its own parameter. This CL fixes that issue by moving the callback to the stack before the object is deleted from the heap. BUG=660847 Review-Url: https://codereview.chromium.org/2784323002 Cr-Commit-Position: refs/heads/master@{#462122} Committed: https://chromium.googlesource.com/chromium/src/+/890cfea7950ebc5910372deb91eb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/890cfea7950ebc5910372deb91eb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
