995599 - services xpcshell tests go beyond localhost

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Patrick McManus [:mcmanus]]

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

services/fxaccounts/tests/xpcshell/test_accounts.js connects to api.accounts.firefox.com and ocsp.digitcert.com. (the latter is probably to check revocation of a cert from the former)

services/sync/tests/unit/test_fxa_startOver.js connects to auth.services.mozilla.com, ocsp.digitcert.com (again, related to a cert, and phx-sync-13-3-4.services.mozilla.com

these should be fixed to be localhost only.

Comment 1

[Nathan Froyd [:froydnj]]

Mark, your name appears prominently as a reviewer and an author of some of these tests.  Any chance we can just set prefs to make the non-local accesses for these go away as easily as bug 999518?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-995599-prevent-services-tests-hitting-the-networ.patch

This patch *seems* to solve the problem, but I'm mainly going on the logs rather than looking at a network trace.  Nathan, is it simple for you to test this patch does indeed solve the problem?  I'll then find a reviewer.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Mark Hammond [:markh] from comment #2)
> This patch *seems* to solve the problem, but I'm mainly going on the logs
> rather than looking at a network trace.  Nathan, is it simple for you to
> test this patch does indeed solve the problem?  I'll then find a reviewer.

No network problems on Linux, one comment on the patch here, though...

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8422247 [details] [diff] [review]
0001-Bug-995599-prevent-services-tests-hitting-the-networ.patch

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

::: services/sync/tests/unit/test_fxa_startOver.js
@@ +27,5 @@
>    // we expect the "legacy" provider (but can't instanceof that, as BrowserIDManager
>    // extends it)
>    do_check_false(Service.identity instanceof BrowserIDManager);
> +
> +  Service.serverURL = "https://localhost/";

I think this will cause problems on our Mac machines if there's not actually a webserver running; bug 1010322 fixes non-existent localhost URLs in other bits of the testsuite.  Setting up a dummy HTTPS webserver might be difficult, though...maybe disable the test on Mac?

I can push to try to see if this instance of localhost works, but it'll be a while before I get those results.

Comment 5

[Nathan Froyd [:froydnj]]

We also crash here:

2:06:42  WARNING -  TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\xpcshell\tests\services\sync\tests\unit\test_service_changePassword.js | test failed (with xpcshell return code: -2147483645), see following log:
12:06:42     INFO -  >>>>>>>
12:06:42     INFO -  [4728] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file c:\\builds\\moz2_slave\\try-w32-d-00000000000000000000\\build\\toolkit\\crashreporter\\nsExceptionHandler.cpp, line 2158
12:06:42     INFO -  System JS : WARNING resource://gre/modules/Preferences.jsm:378 - mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create
12:06:42     INFO -  [4728] WARNING: This method is lossy. Use GetCanonicalPath !: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/xpcom/io/nsLocalFileWin.cpp, line 3348
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | test MAIN run_test pending (1)
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | test run_next_test 0 pending (2)
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | test MAIN run_test finished (2)
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | running event loop
12:06:42     INFO -  TEST-INFO | C:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/test_service_changePassword.js | Starting test_change_password
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | test test_change_password pending (2)
12:06:42     INFO -  Sync.Identity	INFO	Username changed. Removing stored credentials.
12:06:42     INFO -  Sync.Identity	INFO	Basic password has no value. Removing.
12:06:42     INFO -  Sync.Identity	INFO	Sync Key has no value. Deleting.
12:06:42     INFO -  Sync.Identity	INFO	Basic password being updated.
12:06:42     INFO -  Sync.Identity	INFO	Sync Key has no value. Deleting.
12:06:42     INFO -  changePassword() returns false for a network error, the password won't change.
12:06:42     INFO -  Services.Common.RESTRequest	TRACE	HTTP Header authorization: ***** (suppressed)
12:06:42     INFO -  Services.Common.RESTRequest	DEBUG	POST Length: 11
12:06:42     INFO -  Services.Common.RESTRequest	TRACE	POST Body: ILoveJane83
12:06:42     INFO -  System JS : WARNING resource://testing-common/services-common/logging.js:30 - mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create
12:06:42     INFO -  [4728] WARNING: Non-local hostname auth.services.mozilla.com resolved: 1725560127:0: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/netwerk/dns/nsHostResolver.cpp, line 1193
12:06:42     INFO -  Sync.Tracker.Clients	WARN	Changed IDs file clients contains non-object value.
12:06:42     INFO -  Sync.Tracker.Bookmarks	WARN	Changed IDs file bookmarks contains non-object value.
12:06:42     INFO -  Sync.Tracker.Forms	WARN	Changed IDs file forms contains non-object value.
12:06:42     INFO -  Sync.Tracker.History	WARN	Changed IDs file history contains non-object value.
12:06:42     INFO -  Sync.Tracker.Passwords	WARN	Changed IDs file passwords contains non-object value.
12:06:42     INFO -  Sync.Tracker.Addons	WARN	Changed IDs file addons contains non-object value.
12:06:42     INFO -  BAD CONNECT: connecting to auth.services.mozilla.com 0

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-995599-prevent-services-tests-hitting-the-networ.patch

Fix for test_service_changePassword - but note this will also be hitting a non-existent server on localhost as the test starts (this test later creates a server on that port, but the initial test is before the server starts and the comments imply it is specifically checking a network error case)

I'm holding off requesting review until I get clarification this doesn't cause the other issues around non-existing servers on localhost that we briefly discussed on IRC - please let me know when you are confident this is or isn't a problem, and I'll get rnewman to review.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Mark Hammond [:markh] from comment #6)
> I'm holding off requesting review until I get clarification this doesn't
> cause the other issues around non-existing servers on localhost that we
> briefly discussed on IRC - please let me know when you are confident this is
> or isn't a problem, and I'll get rnewman to review.

I haven't gotten a clean run of xpcshell tests on OSX yet, but I do see in the logs that even with this patch, the relevant tests timeout when being run in parallel:

https://tbpl.mozilla.org/php/getParsedLog.php?id=40731121&tree=Try&full=1

This is kind of annoying.  Can you spin up dummy HTTP servers inside the tests instead and just point the tests at those?

Comment 8

[Mark Hammond [:markh] [:mhammond]]

(In reply to Nathan Froyd (:froydnj) from comment #7)

> This is kind of annoying.  Can you spin up dummy HTTP servers inside the
> tests instead and just point the tests at those?

We could, yes.  Some sync tests already do this, but the sync test infra is somewhat opaque, so not a trivial change.

Comment 9

[Richard Newman [:rnewman]]

To clarify this a little bit for the record: the Sync tests themselves all talk to local servers spun up inside tests.

These hits are for the new FxA parts that surround Sync.

See services/sync/tests/unit/head_http_server.js for the server stuff, and services/sync/modules-testing/utils.js (particularly SyncTestingInfrastructure) for how Sync is configured to point to a local test server.

Taking a quick look, test_fxa_startOver seems to be missing some of the configuration steps that SyncTestingInfrastructure applies.

Comment 10

[Nathan Froyd [:froydnj]]

I got some clean OS X xpcshell runs with the patch in this bug and some changes that I needed for bug 101153:

https://tbpl.mozilla.org/?tree=Try&rev=3c1e8a023a01

Hooray!  Mark, can you select an appropriate reviewer for this patch?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8423527 [details] [diff] [review]
0001-Bug-995599-prevent-services-tests-hitting-the-networ.patch

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

A couple of tweaks to prevent the sync xpcshell tests hitting the external network.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8423527 [details] [diff] [review]
0001-Bug-995599-prevent-services-tests-hitting-the-networ.patch

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

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +168,5 @@
>    do_check_eq(result.assertion, credentials.assertion);
>    do_check_eq(result.kB, credentials.kB);
>  
>    // sign out
> +  yield account.signOut(true);

Gentle preference for

  let localOnly = true;
  yield account.signOut(localOnly);

Boolean arguments ftl.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

thanks!  Pushed with suggested change:

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

Nathan, can you please mark as verified when you are satisfied it is?

Comment 14

[:Gijs (he/him)]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d360f03adf66
https://hg.mozilla.org/releases/mozilla-beta/rev/74c01d638458
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9c0864824295
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/2d69e83fc4cf

Comment 16

[Marco Mucci [:MarcoM]]

Hi Florin, can a contact be assigned for QA verification.

Comment 17

[Nathan Froyd [:froydnj]]

This doesn't need QA verification; it is a test-only fix.  The fix works in testing on Try.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This patch was backported to b2g30 in comment 15, but it appears that we're still hitting problems in test_accounts.js with bug 995417 applied:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42378299&tree=Try

Any chance you can help with this Richard since Mark is away? :)

Comment 19

[Richard Newman [:rnewman]]

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

is the patch that added localOnly. I can't imagine that Mark's fix would work without that patch.

Perhaps check that Bug 994934 is on b2g30?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Aha! Thanks :)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Turns out FxA is disabled on B2G v1.4, so I got spenrose's blessing to just disable the FxA tests on b2g30.

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d2607c7b727e