Remove the ability to delay destruction on cancel in the ResourceHandler stack |
||||
Issue descriptionSee issue 659307 for why this is a problem. The only ResourceHandler that currently uses this feature is the RedirectToFileResourceHandler. We'll need to remove this behavior, and add integration tests for the two consumers of this path.
,
Oct 26 2016
Just making sure we're all on the same page on this. IIUC: * The way cancel, once triggered, is currently communicated to the ResourceHandler stack is through OnResponseCompleted() with an appropriate URLRequestStatus. Because that interface allows deferring, cancel can be deferred. * This bug is targeted at changing this communication of Cancel. It will be through a new method on the ResourceHandler (or can I just skip that and just take a delete as a cancel?) which pathway will *not* allow deferring. * This will require making sure that the use cases for RedirectToFileHandler don't require deferring cancellation specifically. Does that fit with your understanding of the goals of this bug? Also, to your knowledge, is RedirectToFileResourceHandler the only handler that defers *cancel* or the only handler that defers OnResponseCompleted() at all? (If you don't know, don't bother to figure it out--I'm going to have to do plenty of digging work like that on this bug anyway.)
,
Oct 26 2016
Ah, I now see issue 659317 . How do you envision the interaction of this bug with that one? Make a separate cancel path that can defer, change this RH so it doesn't, then remove the deferal option to that path?
,
Oct 26 2016
What I'm thinking of is making a new ResourceHandler method: void ResourceHandler::OnCancel(net_error, defer), which for all ResourceHandlers just calls info ResourceHandler::OnResponseCompleted (Or whatever it's called). Once we have the paths separated, then we can modify why RedirectToFileResourceHandler does on cancellation, and remove the defer parameter. I'm open to other paths, this just seemed the most straightforward to me.
,
Oct 26 2016
That sounds awesome, actually. I'll work on figuring out what to do on RedirectToFileHandler while you work on adding that method? Or I can add that method as well? Either way is good by me--that addition seems pretty simple, if it can just go on the ResourceHandler base class for now and be overriden as needed.
,
Oct 26 2016
I'm fine, either way. Not sure how long it will take me to get to it, though. I'll start with auditing existing ResourceHandlers/Throttles for incorrect cancellation (Double cancellation, calling URLRequest::Cancel) and clean that up.
,
Oct 26 2016
Ok. I'll throw together a quick CL to add the Cancel method, then start burrowing into RedirectToFileHandler.
,
Oct 26 2016
Is it reasonable to have a goal (after the paths have been separated) for also removing the net_error argument from OnCancel()? I don't offhand see why it's needed on the RH side (I understand about devtools on the URLRequest side).
,
Oct 26 2016
You mean ResourceController::CancelWithError? For downloads, we want to abort the navigation, which would be ERR_ABORTED. For other random things, I assume we'd want to show an error page, which not be ERR_ABORTED. So I think we need at least two error codes?
,
Oct 26 2016
Or if the downloads path just deletes the ResourceHandler, I suppose that may end up in ERR_ABORTED, anyways? Anyhow, I'm really not sure, and expect that would require auditing a lot of code (Which you'd need to look at anyways, in the process of removing CancelWithError from ResourceController)
,
Oct 26 2016
Ok, CancelWithError is the hook I needed to be reminded of :-}. Ok, I'll explore from there if I'm curious.
,
Apr 18 2017
Matt, I just wanted to check in on your current perspective on this bug. Two general questions: a) I'm not certain if your current refactor for the RH stack supersedes since I think it shifts how cancel is done over to being semi-synchronous, and b) AIUI, we're going to be deprecating RTFRH as part of the servicification effort, which means that at some point in the future this'll be moot anyway. Still worth doing?
,
Apr 18 2017
I've decided to punt the work until we know what's happening with the redirect-to-file resource handler, which we may not know until quite some time. If we do still use ResourceHandlers, I think it'll probably still be worth more refactoring, but may be fewer of them to worry about then.
,
Feb 16 2018
Matt: I'm removing myself from owning this bug. It might be worth your scanning it to see if it should be closed, left alone, or assigned to you. Given servicification, it may well be worth closing.
,
Feb 16 2018
,
May 16 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by mmenke@chromium.org
, Oct 25 2016Status: Assigned (was: Available)