1395837 - Intercepted nsIHttpChannels, when canceled, do not call OnStopRequest

Status: RESOLVED WORKSFORME

(Core :: Networking: HTTP, defect, P2)

Description

[Andrea Marchesini [:baku]]

I and mayhemer discussed this on IRC.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: network_4.patch

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8903477 [details] [diff] [review]
network_4.patch

Review of attachment 8903477 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but except where noted.

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +521,5 @@
>    } else {
>      responseURI = originalURI;
>    }
>  
> +  mChannel->MarkIntercepted();

this change needs a comment.  to be honest, I'm not the best reviewer for this change :/

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3939,5 @@
>  
>          RefPtr<InterceptedChannelChrome> intercepted =
>                  new InterceptedChannelChrome(this, controller, entry);
>          intercepted->NotifyController();
> +        mInterceptedChannel = intercepted;

a blank line before this one please.

@@ +8652,5 @@
>  void
>  nsHttpChannel::MarkIntercepted()
>  {
>      mInterceptCache = INTERCEPTED;
> +    mInterceptedChannel = nullptr;

should you also release it in ReleaseListeners()?  also note that nsHttpChannel can be released (the dtor called) on a different than main thread - hence, if you need to keep the channel referenced until that,make sure you release on the right thread (there is a simple to use mechanism to release on main thread even when nsHttpChannel is released off the main thread).  Although, ReleaseListeners is guarantied to be called on the main thread.

Comment 3

[Firefox Bug Husbandry Bot]

Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 4

[Jason Duell]

Hard for me to know how much to prioritize this---not calling OnStop sounds bad enough (and we have a r+-ish patch) so I'm leaving at P1 for now.

Baku--can you make the tweaks in comment 2 and land this in the 57 time frame?  If not, maybe you can answer the questions and we can find someone to write the patch.

Comment 5

[Andrea Marchesini [:baku]]

Actually, this is not a priority at the moment and I don't want to land it yet: it was needed because of bug 1394102 but I bkelly decided that we don't need that bug (and this one) for 57.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Can you try this again?  I recently landed code moves interception out of nsHttpChannel into a separate InterceptedHttpChannel class in bug 1391693.  I believe canceling it should fire OnStopRequest().

I'm doing further improvement in this area in bug 1204254.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:baku, could you have a look please?

Comment 8

[Andrea Marchesini [:baku]]

Asuth, do we need this bug somehow? I'm not following the ServiceWorker development closely and maybe this code is not needed anymore.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Probably not? I would expect bug 1391693 and bug 1416879 to have addressed this. If they didn't the fix has definitely changed, and with parent intercept happening imminently, it's probably not worth the effort to address this further. Going to resolve this WFM for now and if we need to revisit for bug 1394102 or a related thing, we'll do it there/elsewhere.