New issue
Advanced search Search tips

Issue 659319 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner: ----
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 659317

Blocking:
issue 659307



Sign in to add a comment

Remove the ability to delay destruction on cancel in the ResourceHandler stack

Project Member Reported by mmenke@chromium.org, Oct 25 2016

Issue description

See  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.
 

Comment 1 by mmenke@chromium.org, Oct 25 2016

Owner: rdsmith@chromium.org
Status: Assigned (was: Available)
[rdsmith]:  Assigning this part to you, since you volunteered.  If your plans change, of course, feel free to unassign.
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.)

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?


Comment 4 by mmenke@chromium.org, 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.
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.

Comment 6 by mmenke@chromium.org, 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.
Ok.  I'll throw together a quick CL to add the Cancel method, then start burrowing into RedirectToFileHandler.

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).

Comment 9 by mmenke@chromium.org, 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?
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)
Ok, CancelWithError is the hook I needed to be reminded of :-}.  Ok, I'll explore from there if I'm curious.

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?

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.
Owner: ----
Status: Available (was: Assigned)
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.

Cc: -rdsmith@chromium.org
Status: WontFix (was: Available)

Sign in to add a comment