1236940 - Include the property "ip" in callbacks for WebRequest.onCompleted and .onResponseStarted

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Sébastien Règne]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151208100201

Steps to reproduce:

In background script of a WebExtensions: add a listener on event chrome.webRequest.onCompleted.


Actual results:

The parameter 'ip' is not provide.


Expected results:

The paramter 'ip' contains the server IP adress.

Comment 1

[Andy McKay]

Chatting to Giorgio on IRC, you need to expose the remoteAddress property of https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannelInternal.

Comment 2

[:shell escalante]

not certain who uses - either security or corp network.  do not believe it's a 1.0 blocker based on timing - after connection made.  giorgio is looking into DNS API

Comment 3

[Andrew Swan [:aswan]]

Attachment: Add ip property to chrome.webRequest.onCompleted callback

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8715152 [details] [diff] [review]
Add ip property to chrome.webRequest.onCompleted callback

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

Thanks Andrew. You might try using eslint to get a hang of our style rules. There are instructions here:
https://wiki.mozilla.org/DevTools/CodingStandards

Normally I would r+ this, but I'd like to have a second look at the style stuff. I'll get to it more quickly the next time.

::: toolkit/components/extensions/ext-webRequest.js
@@ +40,5 @@
>          frameId: ExtensionManagement.getFrameId(data.windowId),
>          parentFrameId: ExtensionManagement.getParentFrameId(data.parentWindowId, data.windowId),
>        };
>  
> +      if ('ip' in data) {

We always use double quotes for uniformity.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
@@ +191,5 @@
>      return {};
>    }
>  
> +  let completedUrls = new Set();
> +  function onCompleted(details) {

Any reason you only tested in onCompleted? It looks like there are a couple other methods that support the ip property.

@@ +195,5 @@
> +  function onCompleted(details) {
> +    onRecord("completed", details);
> +
> +    // when resources are cached, the ip property is not present,
> +    // so only check for the ip property the first time around

Capitalize this and end with a period please.

@@ +221,5 @@
> +    if (msg == "skipCompleted") {
> +      checkCompleted = false;
> +      browser.test.sendMessage("ackSkipCompleted");
> +    }
> +    else

The else should go on the same line as the previous brace. Also, you should use braces around the else portion.

::: toolkit/modules/addons/WebRequest.jsm
@@ +337,5 @@
> +      let httpChannel = channel.QueryInterface(Ci.nsIHttpChannelInternal);
> +      try {
> +        data.ip = httpChannel.remoteAddress;
> +      }
> +      catch (e) {

This should go on the same line as the previous brace.

@@ +339,5 @@
> +        data.ip = httpChannel.remoteAddress;
> +      }
> +      catch (e) {
> +        // the remoteAddress getter throws if the address is unavailable.
> +        // but ip is an optional property so just ignore the exception.

Please capitalize sentences in comments.

@@ +341,5 @@
> +      catch (e) {
> +        // the remoteAddress getter throws if the address is unavailable.
> +        // but ip is an optional property so just ignore the exception.
> +      }
> +      

There's some trailing whitespace here. Please remove.

Comment 5

[Andrew Swan [:aswan]]

(In reply to Bill McCloskey (:billm) from comment #4) 
> ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
> @@ +191,5 @@
> >      return {};
> >    }
> >  
> > +  let completedUrls = new Set();
> > +  function onCompleted(details) {
> 
> Any reason you only tested in onCompleted? It looks like there are a couple
> other methods that support the ip property.

Ah, I had meant to circle back to this, thanks for the reminder.
The events for which it is documented are
onBeforeRedirect
onResponseStarted
onCompleted
onErrorOccurred

We don't (yet?) support onErrorOccurred.
For onBeforeRedirect, empirically it looks like the remoteAddress property isn't set on the HTTP channel when we get the event (in any case it's not clear from the documentation, but i guess it is meant to be the address of the server that sent the redirect?)

We certainly can and should handle onResponseStarted though, I'll add that along with the style fixes.

Comment 6

[Andrew Swan [:aswan]]

Attachment: Add ip property to chrome.webRequest.onCompleted callback

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c45fef870db0

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c45fef870db0