Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
164703002:
Don't leak WebHistoryService::Request on HistoryService shutdown. (Closed)
|
Created:
6 years, 10 months ago by davidben Modified:
6 years, 10 months ago CC:
chromium-reviews, browser-components-watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDon't leak WebHistoryService::Request on profile shutdown.
As the profile is shutting down, all URLRequests from that profile must be
destroyed. Instead of having HistoryService leak WebHistoryService's
requests, WebHistoryService maintains expiration requests internally. Those
don't get cancelled unless browser shutdown forces it to happen.
BUG=342164
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252726
Patch Set 1 #
Total comments: 1
Patch Set 2 : Make WebHistoryService return void. #Patch Set 3 : Remnant of previous version. #
Total comments: 2
Patch Set 4 : Style #
Total comments: 6
Patch Set 5 : dubroy comments #
Messages
Total messages: 25 (0 generated)
dubroy: Main review, since it looks like you added the code. sky: OWNERS I do wonder if this call should instead be moved to BrowserDataRemover, with that object holding on to the reference. Would also help with the nuisance right now where the WebHistoryService::Request's callback and success value doesn't figure at all into the ExpireLocalAndRemoteHistoryBetween callback. But this is a smaller change if we want to start with that.
Sorry for the delay. I took a look at this one Friday but forgot to reply to it. On 2014/02/13 21:00:40, David Benjamin wrote: > dubroy: Main review, since it looks like you added the code. > sky: OWNERS > > I do wonder if this call should instead be moved to BrowserDataRemover, with > that object holding on to the reference. Would also help with the nuisance right > now where the WebHistoryService::Request's callback and success value doesn't > figure at all into the ExpireLocalAndRemoteHistoryBetween callback. But this is > a smaller change if we want to start with that. Yeah, I think it might actually be better to return the Request to the BrowsingDataRemover. IIRC, I didn't do that originally just because I wanted to limit the size of the CL.
On 2014/02/17 14:01:13, Patrick Dubroy wrote: > Sorry for the delay. I took a look at this one Friday but forgot to reply to it. > > On 2014/02/13 21:00:40, David Benjamin wrote: > > dubroy: Main review, since it looks like you added the code. > > sky: OWNERS > > > > I do wonder if this call should instead be moved to BrowserDataRemover, with > > that object holding on to the reference. Would also help with the nuisance > right > > now where the WebHistoryService::Request's callback and success value doesn't > > figure at all into the ExpireLocalAndRemoteHistoryBetween callback. But this > is > > a smaller change if we want to start with that. > > Yeah, I think it might actually be better to return the Request to the > BrowsingDataRemover. IIRC, I didn't do that originally just because I wanted to > limit the size of the CL. Urk. So I implemented this variant as we discussed out-of-band (return the Request to the BrowsingDataRemover so it owns it and delay the callback until both halves complete). It doesn't quite work though. The problem is that BrowsingDataRemover itself is /also/ completely unaware of profile shutdown. In the interest of fixing the crash for 34, perhaps we should land and merge this change for now. Then a separate change to delay the callback so the UI is aware of the remote portion, and then file a bug to figure out what to do about BrowsingDataRemover's profile shutdown problems. (Cancel everything, delay shutdown until everything completes, delay shutdown until everything but operations that need network complete, etc.)
LGTM. I think there might be a nicer way to do this, but it would require some refactoring, and we should get this in to fix the bug in M34.
https://codereview.chromium.org/164703002/diff/1/chrome/browser/history/histo... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/164703002/diff/1/chrome/browser/history/histo... chrome/browser/history/history_service.cc:1172: scoped_ptr<history::WebHistoryService::Request> request = This seems like a hack to me. Expiring history doesn't seem like it should be something that is cancelable. That is, you can't really cancel it part way. Seems like WebHistoryService::ExpireHistoryBetween should not have a return value.
On 2014/02/19 17:33:21, sky wrote: > https://codereview.chromium.org/164703002/diff/1/chrome/browser/history/histo... > File chrome/browser/history/history_service.cc (right): > > https://codereview.chromium.org/164703002/diff/1/chrome/browser/history/histo... > chrome/browser/history/history_service.cc:1172: > scoped_ptr<history::WebHistoryService::Request> request = > This seems like a hack to me. Expiring history doesn't seem like it should be > something that is cancelable. That is, you can't really cancel it part way. > Seems like WebHistoryService::ExpireHistoryBetween should not have a return > value. I'm definitely not especially happy with this solution, though my other attempt involved making BrowsingDataRemover own the Request, but BrowsingDataRemover itself leaks on shutdown and is unaware its profile going away. The problem is that the network stack cannot have URLRequests outlive their URLRequestContext. We hit a CHECK() right now if you clear history and exit the browser before the network request completes. Perhaps this whole thing should work differently and things should be written locally such that the browser will retry the network action on restart. I'm not familiar with this code. But this is currently causing shutdown crashes. (I'm sort of puzzled why some many of them, but random samples all point to this operation.) (In case it wasn't clear, this operation doesn't do anything locally. It makes a request over the network.)
What about making WebHistoryService own the request with no return value? On Wed, Feb 19, 2014 at 11:12 AM, <davidben@chromium.org> wrote: > On 2014/02/19 17:33:21, sky wrote: > > https://codereview.chromium.org/164703002/diff/1/chrome/browser/history/histo... >> >> File chrome/browser/history/history_service.cc (right): > > > > https://codereview.chromium.org/164703002/diff/1/chrome/browser/history/histo... >> >> chrome/browser/history/history_service.cc:1172: >> scoped_ptr<history::WebHistoryService::Request> request = >> This seems like a hack to me. Expiring history doesn't seem like it should >> be >> something that is cancelable. That is, you can't really cancel it part >> way. >> Seems like WebHistoryService::ExpireHistoryBetween should not have a >> return >> value. > > > I'm definitely not especially happy with this solution, though my other > attempt > involved making BrowsingDataRemover own the Request, but BrowsingDataRemover > itself leaks on shutdown and is unaware its profile going away. > > The problem is that the network stack cannot have URLRequests outlive their > URLRequestContext. We hit a CHECK() right now if you clear history and exit > the > browser before the network request completes. Perhaps this whole thing > should > work differently and things should be written locally such that the browser > will > retry the network action on restart. I'm not familiar with this code. But > this > is currently causing shutdown crashes. (I'm sort of puzzled why some many of > them, but random samples all point to this operation.) > > (In case it wasn't clear, this operation doesn't do anything locally. It > makes a > request over the network.) > > https://codereview.chromium.org/164703002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/19 21:16:56, sky wrote: > What about making WebHistoryService own the request with no return value? > That could probably also work, though then the caller needs other means to nullify the callback; BrowserHistoryHandler (chrome/browser/ui/webui/history_ui.cc) currently binds the callback with a base::Unretained(this) and instead uses the return value.
Are we using the callback anywhere? On Wed, Feb 19, 2014 at 1:28 PM, <davidben@chromium.org> wrote: > On 2014/02/19 21:16:56, sky wrote: >> >> What about making WebHistoryService own the request with no return value? > > > > That could probably also work, though then the caller needs other means to > nullify the callback; BrowserHistoryHandler > (chrome/browser/ui/webui/history_ui.cc) currently binds the callback with a > base::Unretained(this) and instead uses the return value. > > https://codereview.chromium.org/164703002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
How's this version?
LGTM https://codereview.chromium.org/164703002/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/164703002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:390: : pending_delete_requests_(0), weak_factory_(this) { nit: one param per line (when you wrap).
https://codereview.chromium.org/164703002/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/164703002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:390: : pending_delete_requests_(0), weak_factory_(this) { On 2014/02/19 23:20:10, sky wrote: > nit: one param per line (when you wrap). Done.
On 2014/02/19 23:21:20, David Benjamin wrote: > https://codereview.chromium.org/164703002/diff/220001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/history_ui.cc (right): > > https://codereview.chromium.org/164703002/diff/220001/chrome/browser/ui/webui... > chrome/browser/ui/webui/history_ui.cc:390: : pending_delete_requests_(0), > weak_factory_(this) { > On 2014/02/19 23:20:10, sky wrote: > > nit: one param per line (when you wrap). > > Done. dubroy: Could take another look since the CL's changed a bit? Thanks!
https://codereview.chromium.org/164703002/diff/300001/chrome/browser/history/... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/164703002/diff/300001/chrome/browser/history/... chrome/browser/history/history_service.cc:1171: base::Bind(&ExpireWebHistoryComplete)); I'd just use base::DoNothing() here. https://codereview.chromium.org/164703002/diff/300001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/164703002/diff/300001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:535: if (delete_task_tracker_.HasTrackedTasks() || See comment below -- the deletion should also fail if the web history deletion is still outstanding. https://codereview.chromium.org/164703002/diff/300001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:618: pending_delete_requests_++; This seems to imply that there can be multiple deletions outstanding, which shouldn't be the case. I think this should instead be a boolean, and the check above should not allow a new deletion to begin while the web history deletion is still outstanding.
https://codereview.chromium.org/164703002/diff/300001/chrome/browser/history/... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/164703002/diff/300001/chrome/browser/history/... chrome/browser/history/history_service.cc:1171: base::Bind(&ExpireWebHistoryComplete)); On 2014/02/20 12:56:55, Patrick Dubroy wrote: > I'd just use base::DoNothing() here. Hrm. Does that work? There's an extra argument to be discarded. https://codereview.chromium.org/164703002/diff/300001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/164703002/diff/300001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:535: if (delete_task_tracker_.HasTrackedTasks() || On 2014/02/20 12:56:55, Patrick Dubroy wrote: > See comment below -- the deletion should also fail if the web history deletion > is still outstanding. Done. And added a TODO because it doesn't look like history.js quite handles this case correctly. (If the user tries to delete one set of entries while the previous delete request is still running.) https://codereview.chromium.org/164703002/diff/300001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_ui.cc:618: pending_delete_requests_++; On 2014/02/20 12:56:55, Patrick Dubroy wrote: > This seems to imply that there can be multiple deletions outstanding, which > shouldn't be the case. I think this should instead be a boolean, and the check > above should not allow a new deletion to begin while the web history deletion is > still outstanding. > Done.
lgtm
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/164703002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
On 2014/02/21 22:59:23, I haz the power (commit-bot) wrote: > Retried try job too often on win_rel for step(s) browser_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Looks like swarming fell over or something? Trying again.
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/164703002/360001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/164703002/360001
Message was sent while issue was closed.
Change committed as 252726 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
