Enable interception of pings through service workers

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla40
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
This is similar to bug 1147695 but for pings.
(Assignee)

Comment 1

3 years ago
Created attachment 8584017 [details] [diff] [review]
Enable interception of pings through service workers

Currently when sending a beacon, HttpBaseChannel::ShouldIntercept tries
to get access to the nsINetworkInterceptController interface through the
channel's notification callbacks, but in this case the notification
callback is the nsPingListener object.

This patch extends nsPingListener to make it aware of
nsINetworkInterceptController, and have it route the request for
nsINetworkInterceptController correctly to the docshell without the need
to mess with the notification callbacks.

This will be tested in bug 1147699.
(Assignee)

Updated

3 years ago
Attachment #8584017 - Flags: review?(nsm.nikhil)
Comment on attachment 8584017 [details] [diff] [review]
Enable interception of pings through service workers

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

Please pass this by someone who knows docshell once.

::: docshell/base/nsDocShell.cpp
@@ +531,5 @@
> +  if (aIID.Equals(NS_GET_IID(nsINetworkInterceptController)) &&
> +      mInterceptController) {
> +    nsCOMPtr<nsINetworkInterceptController> copy(mInterceptController);
> +    *aResult = copy.forget().take();
> +    return NS_OK;

Why not use the same pattern as for channel event sink?
Attachment #8584017 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 3

3 years ago
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #2)
> ::: docshell/base/nsDocShell.cpp
> @@ +531,5 @@
> > +  if (aIID.Equals(NS_GET_IID(nsINetworkInterceptController)) &&
> > +      mInterceptController) {
> > +    nsCOMPtr<nsINetworkInterceptController> copy(mInterceptController);
> > +    *aResult = copy.forget().take();
> > +    return NS_OK;
> 
> Why not use the same pattern as for channel event sink?

Because I'd prefer us to move away from calling AddRef and Release manually...  But you have a good point on the inconsistency, so I'll change the other pattern.  :-)
(Assignee)

Comment 4

3 years ago
Created attachment 8585103 [details] [diff] [review]
Enable interception of pings through service workers

Currently when sending a beacon, HttpBaseChannel::ShouldIntercept tries
to get access to the nsINetworkInterceptController interface through the
channel's notification callbacks, but in this case the notification
callback is the nsPingListener object.

This patch extends nsPingListener to make it aware of
nsINetworkInterceptController, and have it route the request for
nsINetworkInterceptController correctly to the docshell without the need
to mess with the notification callbacks.

This will be tested in bug 1147699.
Attachment #8584017 - Attachment is obsolete: true
Attachment #8585103 - Flags: review?(bugs)

Comment 5

3 years ago
Comment on attachment 8585103 [details] [diff] [review]
Enable interception of pings through service workers

Could we mark SendPingInfo to be stack class?
Attachment #8585103 - Flags: review?(bugs) → review+
(Assignee)

Comment 6

3 years ago
Of course!
https://hg.mozilla.org/mozilla-central/rev/aca9840cf06f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.