Closed
Bug 1236940
Opened 9 years ago
Closed 9 years ago
Include the property "ip" in callbacks for WebRequest.onCompleted and .onResponseStarted
Categories
(WebExtensions :: Untriaged, defect, P2)
Tracking
(firefox47 fixed)
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: regseb, Assigned: aswan)
References
Details
(Whiteboard: [good first bug] triaged)
Attachments
(1 file, 1 obsolete file)
6.56 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
Blocks: webRequest-full
Updated•9 years ago
|
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Updated•9 years ago
|
Flags: blocking-webextensions?
Priority: -- → P2
Whiteboard: triaged
Updated•9 years ago
|
Assignee: nobody → aswan
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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
Flags: blocking-webextensions? → blocking-webextensions-
Whiteboard: triaged → [good first bug] triaged
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8715152 -
Flags: review?(wmccloskey)
Assignee | ||
Updated•9 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8715152 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 5•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Summary: Webrequest onCompleted not provide IP → Include the property "ip" in callbacks for WebRequest.onCompleted and .onResponseStarted
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8717615 -
Flags: review?(wmccloskey)
Assignee | ||
Updated•9 years ago
|
Attachment #8715152 -
Attachment is obsolete: true
Attachment #8717615 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Comment 8•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•