1395140 - Implement "http-on-stop-request" notification

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Andrea Marchesini [:baku]]

This notification is needed to abort a Request.signal object when the channel is canceled.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1378342#c31

Comment 1

[Andrea Marchesini [:baku]]

Attachment: network_1.patch

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8902675 [details] [diff] [review]
network_1.patch

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

This looks good!  I just want to check the final version since few updates are needed.

(One note, channels that redirects to a different URI, both nsHttpChannel and HttpChannelChild, don't call OnStart/OnStopRequest on it's listener)


You missed one place you must also call your new topic: HttpBaseChannel::DoNotifyListener()

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1197,5 @@
>    }
>    mCacheEntryAvailable = false;
>  
> +  // notify "http-on-stop-connect" observers
> +  gHttpHandler->OnStopRequest(this);

this is the right method and more or less the right place, but I would prefer to do this call before the call to ReleaseListeners() just few lines above (out of context here) and definitely before removing from the load group (what you do here...

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7548,5 @@
>  
>      ReleaseListeners();
>  
> +    // notify "http-on-stop-connect" observers
> +    gHttpHandler->OnStopRequest(this);

...but not here).  do this after you've called mListener->OnStopRequest (out of the if (mListener) branch of course).  we must be consistent in what state the channel is (what references it still keeps) when your topic is fired.

::: netwerk/protocol/http/nsIHttpProtocolHandler.idl
@@ +133,5 @@
>  #define NS_HTTP_ON_USERAGENT_REQUEST_TOPIC "http-on-useragent-request"
>  
> +/**
> + * When an HTTP request is terminated, succeeding or failing, this observer
> + * topic is notified.

write it as:

This topic is notified for every http channel right after it called OnStopRequest on its listener, regardless whether it was finished successfully, failed or has been canceled.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: network_1.patch

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8903476 [details] [diff] [review]
network_1.patch

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

Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9d4d7a4914
Implement "http-on-stop-request" notification, r=mayhemer

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d9d4d7a4914