Closed Bug 1395140 Opened 4 years ago Closed 4 years ago

Implement "http-on-stop-request" notification

Categories

(Core :: Networking: HTTP, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

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
Attached patch network_1.patch (obsolete) — Splinter Review
Attachment #8902675 - Flags: review?(honzab.moz)
Blocks: 1394102
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.
Attachment #8902675 - Flags: review?(honzab.moz) → review-
Attached patch network_1.patchSplinter Review
Attachment #8902675 - Attachment is obsolete: true
Attachment #8903476 - Flags: review?(honzab.moz)
Blocks: 1395837
Comment on attachment 8903476 [details] [diff] [review]
network_1.patch

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

Thanks!
Attachment #8903476 - Flags: review?(honzab.moz) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9d4d7a4914
Implement "http-on-stop-request" notification, r=mayhemer
Whiteboard: [necko-active]
https://hg.mozilla.org/mozilla-central/rev/8d9d4d7a4914
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.