995806 - test_DirectoryLinksProvider.js xpcshell test makes tcp connection to example.com

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Patrick McManus [:mcmanus]]

The test infrastrucutre is meant to be self contained - so xpcshell, mochitests, and crashtests need to be self hosted.

toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js actually makes a connection via nsichannel to example.com 93.184.216.119 every time the test is executed.

this should be fixed to be localhost only.

Comment 1

[Nathan Froyd [:froydnj]]

Drew, your name is on this file as the primary reviewer.

Can the test just request things from the local http server that gets set up for the test?  Can you find someone to make that change or other necessary changes?

Comment 2

[Drew Willcoxon :adw]

Olivier, Maxim, Marina, would you be able to look at this?  The example.com URLs in the test are all tile/site sources, not links metadata sources, so I don't see how they end up getting loaded.

Comment 3

[maxim zhilyaev]

Drew, I think example.com URLs are never loaded, they are always a part of the data section of the data url constructed by the test as in:

  let data = {
    "en-US": [{url: "http://example.com", title: "US"}],
    "zh-CN": [
              {url: "http://example.net", title: "CN"},
              {url:"http://example.net/2", title: "CN2"}
    ],
  };
  let dataURI = 'data:application/json,' + JSON.stringify(data);

Let me know if understood your concern correctly and addressed it

Comment 4

[Drew Willcoxon :adw]

Yes, that's what it looks like to me, too, but apparently something in the test is actually loading example.com.  I thought I might have overlooked something that one of you might see.

Comment 5

[Nathan Froyd [:froydnj]]

Finally took at closer look at this and got:

 0:03.92 [31042] WARNING: Non-local hostname nosuchhost resolved: 2218907459:0: file /home/froydnj/src/gecko-dev.git/netwerk/dns/nsHostResolver.cpp, line 1193
 0:03.92 BAD CONNECT: connecting to nosuchhost 0
 0:03.92 Hit MOZ_CRASH(Attempting to connect to non-local address!) at /home/froydnj/src/gecko-dev.git/netwerk/base/src/nsSocketTransport2.cpp:1190

which is coming from here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js#208

This is one of the annoying things about disabling non-local connections: xpcshell has somewhat different behavior than the browser in this case.  (The browser returns nice errors back to the HTML page that spawned the request...sometimes.)

Patrick, do you have any networking insight into what we can do here?

Comment 6

[Nathan Froyd [:froydnj]]

And we hit it here, too:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js#289

I'm assuming that we'd rather not disable these bits of the test if we can help it.

Comment 7

[maxim zhilyaev]

These are negative tests that ensure that bad urls and unresolved hosts are handled correctly.
Patric, what would you suggest for an appropriate URL and hostname for such tests (a non-existent IP, perhaps)?

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to maxim zhilyaev from comment #7)
> These are negative tests that ensure that bad urls and unresolved hosts are
> handled correctly.
> Patric, what would you suggest for an appropriate URL and hostname for such
> tests (a non-existent IP, perhaps)?

yes - if you use a hostname that doesn't resolve at all then I wouldn't expect a problem as I remember the enforcement patch. (iirc it asserts when trying to connect to a non-localhost IP and only warns on trying to look up names other than localhost).

something like nosuchhost.localhost. ought to be pretty safe

Comment 9

[Ed Lee :Mardak]

maksik, are you able to test this locally to see if a change would fix this problem?

Comment 10

[maxim zhilyaev]

Attachment: v1. avoiding external DNS lookups

Tested with tcpdump on local machine whiel running the test

>> tcpdump -vvv -s 0 -l -n port 53

10:16:11.132372 IP (tos 0x0, ttl 255, id 52101, offset 0, flags [none], proto UDP (17), length 66)
    10.252.26.250.49167 > 10.252.75.5.53: [udp sum ok] 19606+ A? nosuchhost.localhost. (38)
10:16:11.135500 IP (tos 0x0, ttl 63, id 34130, offset 0, flags [none], proto UDP (17), length 107)
    10.252.75.5.53 > 10.252.26.250.49167: [udp sum ok] 19606 NXDomain* q: A? nosuchhost.localhost. 0/1/0 ns: localhost. [1d] SOA localhost. root.localhost. 42 10800 900 604800 86400 (79)

network utility show local IP as 10.252.26.250

So, the fix appears to work.

Comment 11

[Ed Lee :Mardak]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d34e5a9ec24

Comment 13

[Ryan VanderMeulen [:RyanVM]]

maxim, we'd like to backport bug 995417 to mozilla-beta but are running into this bug in the process. However, this patch doesn't apply cleanly to that branch. Can you please post a branch patch when you get a chance? :)

Thanks!

Comment 14

[maxim zhilyaev]

Attachment: remove http request to non-local host in mozilla-beta

Rayn, the original patch is not applicable to mozilla-beta since mozilla-beta does not have functionality that the patched test cases are testing.  However, there's an offending line that issues a non-local http request found here:
https://github.com/mozilla/gecko-dev/blob/beta/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js#L130

I am attaching a patch that fixes this issue.
Please note that this code will is removed in master

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Marvelous, thank you!
https://hg.mozilla.org/releases/mozilla-beta/rev/5c047cecbe5a