New issue
Advanced search Search tips

Issue 833613 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

DevToolsURLLoaderInterceptor InterceptionJob has no comments

Project Member Reported by mmenke@chromium.org, Apr 16 2018

Issue description

I have a CL that breaks DevToolsURLLoaderInterceptor, so I figured I'd just need to update DevToolsURLLoaderInterceptor to be compatible with the slightly modified URLLoader API.

Looking at the DevTools code, though, it's pretty much completely uncommented, which makes figuring out how it works rather difficult.

Could we please get some comments for those classes?
 

Comment 1 by caseq@chromium.org, Apr 16 2018

This is going away with the transition to the network service, hopefully not too far in the future. So not sure if it's worth investing the time to document this particular one. If you have any specific questions, I'll be happy to answer those -- or perhaps I would be able to help with the transition to the new API.

Comment 2 by mmenke@chromium.org, Apr 16 2018

I don't understand its lifetime semantics.  I've modified URLLoader so it deletes itself when done, and apparently this broke InterceptionJob (https://chromium-review.googlesource.com/c/chromium/src/+/1012455).

Closing client_binding_ in InterceptionJob::OnComplete() seems to work, but I have no idea if this could cause a leak of the InterceptionJob (What if it gets a truncated body, for instance?)

Comment 3 by mmenke@chromium.org, Apr 16 2018

I think the issue is that the URLLoader completes, tells the interception job, and deletes itself.  The InterceptionJob sees the closed client pipe (But isn't watching the data pipe...Or maybe it does?), and calls shutdown, even though it already got the complete event, so doesn't really need anything else from that channel.

So if I just stop watching the client channel, things work...  But it, say, the data pipe breaks before the body is read, for some reason, I have no idea what will happen to the object.

Comment 4 by caseq@chromium.org, Apr 16 2018

Ouch, sorry, I mis-read the original description, the URLLoader-based one is not going away! I'll add some description of life-cycle and life times there.

So when the data pipe breaks, the BodyReader is supposed to be notified by the DataPipeDrainer and ultimately invoke InterceptionJob::ResponseBodyComplete().

There are two options there, either we're still waiting for the [DevTools protocol] client to decide on what to do, in which case we keep waiting, or we just send whatever we've got to the URLLoaderClient and invoke Shutdown() (which will self-destroy). So closing the client binding in OnComplete() that you're adding actually makes perfect sense, though I would perhaps also add loader_.reset() there for consistency.

Owner: caseq@chromium.org
Status: Assigned (was: Untriaged)

Sign in to add a comment