Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2536003003:
MD Settings: Fix WebUI lifecycle issues in Clear Browsing Data handler. (Closed)
DescriptionMD Settings: Fix WebUI lifecycle issues in Clear Browsing Data handler.
BUG=649644
Committed: https://crrev.com/ff49185dcc63d419a850902ed5220377701d1e3c
Cr-Commit-Position: refs/heads/master@{#435353}
Patch Set 1 #
Total comments: 6
Patch Set 2 : address dbeam comments #Messages
Total messages: 15 (9 generated)
tommycli@chromium.org changed reviewers: + dbeam@chromium.org
dbeam: PTAL webui lifecycle issues. annotations below. thanks! https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (left): https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:262: task_observer_.reset(); This was not necessary right? Reading the code -- it appears that this can only be set by the HandleClearBrowsingData handler, which should come after the HandleInitialize handler. After that, any refreshes or navigations away will trigger OnJavascriptDisallowed right? https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:124: counters_.clear(); This was the missing line that caused the crashes.
lgtm https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (left): https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:262: task_observer_.reset(); On 2016/11/29 01:08:12, tommycli wrote: > This was not necessary right? Reading the code -- it appears that this can only > be set by the HandleClearBrowsingData handler, which should come after the > HandleInitialize handler. > > After that, any refreshes or navigations away will trigger > OnJavascriptDisallowed right? in theory, though a renderer crash might affect this. related: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/md_downloads/md_... https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:113: DCHECK(counters_.empty()); nit: I suppose this is OK, but why can't we just .clear() here as well?
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
dbeam: Thanks! https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (left): https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:262: task_observer_.reset(); On 2016/11/29 06:18:58, Dan Beam wrote: > On 2016/11/29 01:08:12, tommycli wrote: > > This was not necessary right? Reading the code -- it appears that this can > only > > be set by the HandleClearBrowsingData handler, which should come after the > > HandleInitialize handler. > > > > After that, any refreshes or navigations away will trigger > > OnJavascriptDisallowed right? > > in theory, though a renderer crash might affect this. > > related: > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/md_downloads/md_... I see. I restored it and added a comment. https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2536003003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:113: DCHECK(counters_.empty()); On 2016/11/29 06:18:58, Dan Beam wrote: > nit: I suppose this is OK, but why can't we just .clear() here as well? Clearing would be also harmless, but clearing tells devs "this list might have something in it", whereas this dcheck tells devs "this list should definitely be empty".
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 tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2536003003/#ps20001 (title: "address dbeam comments")
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": 20001, "attempt_start_ts": 1480530968878810,
"parent_rev": "130f9c7ec47dce2f361b478fb3e051359ae480ea", "commit_rev":
"4a212d91185454feb5f4a8a0e2c468c8b8de10b7"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Fix WebUI lifecycle issues in Clear Browsing Data handler. BUG=649644 ========== to ========== MD Settings: Fix WebUI lifecycle issues in Clear Browsing Data handler. BUG=649644 Committed: https://crrev.com/ff49185dcc63d419a850902ed5220377701d1e3c Cr-Commit-Position: refs/heads/master@{#435353} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ff49185dcc63d419a850902ed5220377701d1e3c Cr-Commit-Position: refs/heads/master@{#435353} |
