907770 - HTTPS hosts don't work in Mochitests on Android/B2G

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Hosts like https://example.com don't seem to work when used in a Mochitest on Android. This is not high priority though; just something I ran into when writing the test in bug 901085.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

we need to fix ssltunnel or the parameters we send to it.  This was something we didn't address 3 years ago when we started testing remotely.

Comment 2

[Martijn Wargers (dead)]

content/base/test/test_CrossSiteXHR_origin.html also suffers from this bug, I think.

Comment 3

[Martijn Wargers (dead)]

And dom/permission/tests/test_permission_basics.html

Comment 4

[Martijn Wargers (dead)]

David Clarke looked into this, what he told me is that this is a problem with the ssltunnel process not being run on the device/emulator:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#806

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

this is a known issue and somebody just needs to spend a day figuring it out and testing it.

Comment 6

[(not currently active) Ted Mielczarek]

We would be running ssltunnel on the same machine where we run xpcshell with httpd.js, FWIW. It's probably just not being configured or executed in the remote harnesses.

Comment 7

[[e-mail] [:onecyrenus]]

ted yes, currently the build process builds ssltunnel for the remote device, however we try to execute it on the local device. 
ssltunnel.cfg is configured in the profile dir, unclear if that is getting copied over to the remote harness. but yes we just have to refactor that code that in remote environments we run the process through adb.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

I am unclear if this is Android or B2G?

On android the host computer (linux) runs xpcshell+httpd.js.  We have the host machine utilities and binaries available and we should be running ssltunnel on the host machine.  We would need to configure it to listen on ip:port and forward to the correct ip:port.  This should be trivial.  On B2G, I am not sure what to make of it.  I believe on B2G we do a similar thing where the host machine is serving web pages but using adb to communicate with the device/emulator.

Comment 9

[Martijn Wargers (dead)]

Oh, ok, sorry, I thought that Android and B2g would need the same solution.
I wonder if the patch for bug 466524 could also be used for mochitests.

Comment 10

[[e-mail] [:onecyrenus]]

Joel, 
   Just an fyi, ssltunnel is built for the remote device, as in the executable is only executable on the remote device.  Somehow you would have to change ssltunnel so it is built for the host machine. this is why the execute fails.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

we already use xpcshell for the host machine and in the same package we have ssltunnel.  Please reference the host machine bits.  This is how the android tests work and from my understanding the b2g tests.

Either we need two bugs or all comments should reference which test flavor they are talking about.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

What is the path forward here?  Not testing any SSL stuff on b2g is ... unfortunate.

Comment 14

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-907770-Making-ssltunnel-run-for-mochitest-remote.patch

(From Bug 975149)  I started looking at this.  Running ssltunnel is a little gnarly because of the way that the remote tests are built, but this does the job.  Actually having it run and work is less good.  I couldn't get my code working with these changes, despite everything being in place (to my knowledge).

Comment 15

[Martin Thomson [:mt:]]

OK, now I'm sad.  The above works for content/base/test/test_CrossSiteXHR.html

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I have a feeling bug 676972 has the same root cause or at least is very similar.

Comment 17

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-907770-Making-ssltunnel-run-for-mochitest-remote.patch

Somehow, this feels too hacky, but it does work.  I suspect that someone with better familiarity with this code than I will want to take a look at build/automation.py.in and testing/mochitest/runtests.py.

Things I noticed is that there is a lot of code duplication between the two of these.  And I found the way that the runtestsremote.py manages configuration to be a little scary.

Comment 18

[(not currently active) Ted Mielczarek]

(In reply to Martin Thomson [:mt] from comment #17)
> Things I noticed is that there is a lot of code duplication between the two
> of these.  And I found the way that the runtestsremote.py manages
> configuration to be a little scary.

Yes, we refactored runtests.py to not rely on automation.py, in the process duplicating some code out of it. However, there are still things using automation.py (including runtestsremote.py, for other unfortunate reasons).

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8383422 [details] [diff] [review]
0001-Bug-907770-Making-ssltunnel-run-for-mochitest-remote.patch

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

::: testing/mochitest/runtestsremote.py
@@ +569,5 @@
>          # whereas runtest.py's `runApp` takes a mozprofile object.
>          if 'profileDir' not in kwargs and 'profile' in kwargs:
>              kwargs['profileDir'] = kwargs.pop('profile').profile
> +        # currently, we can't trust automation.py to generate the right trust anchors
> +        # in the certificate DB

I do not understand this comment. It would be great to get a more complete explanation in this comment, or to reference something a bug or comment in a bug that explains it in more detail.

Comment 20

[Martin Thomson [:mt:]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18)
> (In reply to Martin Thomson [:mt] from comment #17)
> > Things I noticed is that there is a lot of code duplication between the two
> > of these.  And I found the way that the runtestsremote.py manages
> > configuration to be a little scary.
> 
> Yes, we refactored runtests.py to not rely on automation.py, in the process
> duplicating some code out of it. However, there are still things using
> automation.py (including runtestsremote.py, for other unfortunate reasons).

Ahh, so that's the direction of movement.  I wasn't sure.  That helps.  I was going to refactor automation.py to fix some of the issues I saw there, but now I wont.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #19)
> ::: testing/mochitest/runtestsremote.py
> @@ +569,5 @@
> >          # whereas runtest.py's `runApp` takes a mozprofile object.
> >          if 'profileDir' not in kwargs and 'profile' in kwargs:
> >              kwargs['profileDir'] = kwargs.pop('profile').profile
> > +        # currently, we can't trust automation.py to generate the right trust anchors
> > +        # in the certificate DB
> 
> I do not understand this comment. It would be great to get a more complete
> explanation in this comment, or to reference something a bug or comment in a
> bug that explains it in more detail.

Will do.  I hope to be able to remove the problem entirely.  But I don't understand all the context yet.  As I said, the code here is frightening.

Comment 21

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-907770-Making-ssltunnel-run-for-mochitest-remote.patch

This works for me, though with an old and slow android machine, I still get tests timing out - that's the fault of the tests though.

I have, and will attach, some more patches for some of the places where I know tests to have been disabled due to lack of https.  I'm not inclined to do the full search, I have no interest in fighting any more losing battles on this.  I'll try to catch those that were noted here at least.

Comment 22

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-907770-Enabling-cross-origin-tests-for-Android.patch

Cross origin tests in content/base enabled.  WFM.

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

test_user_agent_overrides intentionally fails if https is working for Android.

To correct that, |skipHttps| should be set to false here,

http://mxr.mozilla.org/mozilla-central/source/netwerk/test/mochitests/test_user_agent_overrides.html?rev=a8711da4abee&force=1#107

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Are these patches ready to be reviewed?

Comment 25

[Martin Thomson [:mt:]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #24)
> Are these patches ready to be reviewed?

I'm reluctant to ask for a review due to the large number of breakages in try.  I think that the patch is OK, but it's only the first of many.  That is, unless people are happy for me to defer fixing the failures.  I'd want to at least take a good look at some before doing that in any case. 

I've been at the IETF meeting, so haven't had time to fix the breakages.

Comment 26

[Martin Thomson [:mt:]]

OK, this is ready for Android.  More work is required to get things working on B2G.

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

I have a patch for the one failed test there (yeah, comment #23) that I've tested locally.

Comment 27

[Martin Thomson [:mt:]]

Attachment: Making ssltunnel run for mochitest-remote

Comment 28

[Martin Thomson [:mt:]]

Attachment: Enabling cross origin tests for Android.

Comment 29

[Martin Thomson [:mt:]]

Attachment: Enabling dom/permission/tests/test_permission_basics.html

Comment 30

[Martin Thomson [:mt:]]

Attachment: Restoring https test.

Comment 31

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8390716 [details] [diff] [review]
Making ssltunnel run for mochitest-remote

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

this patch cleans up a lot of little things, I like it in general and it simplifies a few things.  I assume this has been tested on try server for all mochitest configurations and platforms?

::: testing/mochitest/runtests.py
@@ +818,5 @@
>      del self.profile
> +    if options.pidFile != "":
> +      try:
> +        os.remove(options.pidFile)
> +        os.remove(options.pidFile + ".xpcshell.pid")

will we always havea .xpcshell.pid file, if not, this warning will be printed all the time.

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8390719 [details] [diff] [review]
Restoring https test.

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

thanks!

Comment 33

[Martin Thomson [:mt:]]

OK, this demonstrates how messed up my understanding is here.  This worked fine, I thought I could land a fix for android, then later one for b2g.  This is not the case.  I am clearly a complete novice with Python because a basic object/dict mixup in the b2g code eluded me for a week.  Now all I need to do is chase down how to update the xre bundle, because that doesn't include ssltunnel for some reason.

Once that is done, this is good to go (though I'll need an updated review for the b2g changes).

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

Comment 34

[Geoff Brown [:gbrown]]

(In reply to Martin Thomson [:mt] from comment #33)
> Now all I
> need to do is chase down how to update the xre bundle, because that doesn't
> include ssltunnel for some reason.

tegra-host-utils.Linux.742597.zip (used for all the android tests) contains bin/ssltunnel -- is that what you need?

Comment 35

[Martin Thomson [:mt:]]

(In reply to Geoff Brown [:gbrown] from comment #34)
> (In reply to Martin Thomson [:mt] from comment #33)
> > Now all I
> > need to do is chase down how to update the xre bundle, because that doesn't
> > include ssltunnel for some reason.
> 
> tegra-host-utils.Linux.742597.zip (used for all the android tests) contains
> bin/ssltunnel -- is that what you need?

This would be fine if b2g did not use a completely different bundle.  I want to make sure that there isn't some special treatment their bundle has that the above does not.

Comment 36

[Martin Thomson [:mt:]]

Comment on attachment 8390718 [details] [diff] [review]
Enabling dom/permission/tests/test_permission_basics.html

This test needs further work.

Comment 37

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-907770-Making-ssltunnel-run-for-mochitest-remote.patch

Carrying forward r+ from jmaher.  Unbitrot, review comments.

Comment 38

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-907770-Fixing-b2g-test-runner.patch

New content in this patch to deal with b2g failures.  Will probably squash with 0001, but want to keep the review load manageable.  Can't land this until Bug 987406 lands.

Comment 39

[Martin Thomson [:mt:]]

Attachment: 0003-Bug-907770-Enabling-cross-origin-tests-for-Android.patch

Carrying r+ from jmaher.

Comment 40

[Martin Thomson [:mt:]]

Attachment: 0004-Bug-907770-Restoring-https-test.patch

Carrying r+ from jmaher.

Comment 41

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8397232 [details] [diff] [review]
0002-Bug-907770-Fixing-b2g-test-runner.patch

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

I think you are close here.

::: testing/mochitest/runtests.py
@@ +529,5 @@
> +    # httpd-path is specified by standard makefile targets and may be specified
> +    # on the command line to select a particular version of httpd.js. If not
> +    # specified, try to select the one from hostutils.zip, as required in bug 882932.
> +    if not options.httpdPath:
> +      options.httpdPath = os.path.join(options.utilityPath, "components")

isn't httpd.js in the script directory?  It is in the components directory, but it should be the same as what is in the script directory.

@@ +812,5 @@
> +    if self._locations is not None:
> +      return self._locations
> +    locations_file = os.path.join(SCRIPT_DIR, 'server-locations.txt')
> +    self._locations = ServerLocations(locations_file)
> +    return self._locations

why is this duplicated?

::: testing/mochitest/runtestsb2g.py
@@ -213,5 @@
> -        if (options.pidFile != ""):
> -            f = open(options.pidFile + ".xpcshell.pid", 'w')
> -            f.write("%s" % self.server._process.pid)
> -            f.close()
> -        self.server.ensureReady(90)

does startServers handle the pidfile stuff? I don't see it in my copy on m-c.

Comment 42

[Martin Thomson [:mt:]]

(In reply to Joel Maher (:jmaher) from comment #41)
> ::: testing/mochitest/runtests.py
> @@ +529,5 @@
> > +    # httpd-path is specified by standard makefile targets and may be specified
> > +    # on the command line to select a particular version of httpd.js. If not
> > +    # specified, try to select the one from hostutils.zip, as required in bug 882932.
> > +    if not options.httpdPath:
> > +      options.httpdPath = os.path.join(options.utilityPath, "components")
> 
> isn't httpd.js in the script directory?  It is in the components directory,
> but it should be the same as what is in the script directory.

It's not the same.  Or at least, it's not same enough.  The one that is in the scripts directory does not work for android (and I'm assuming b2g as well).  It wasn't just one thing, it seemed systemically busted.  I tried diagnosing the problems, but it only got more broken with every "fix" I added.
 
> @@ +812,5 @@
> > +    if self._locations is not None:
> > +      return self._locations
> > +    locations_file = os.path.join(SCRIPT_DIR, 'server-locations.txt')
> > +    self._locations = ServerLocations(locations_file)
> > +    return self._locations
> 
> why is this duplicated?

A big fat mistake.  I needed to move it from Mochitests to the mixin and I guess I didn't delete it.  Thanks for catching that.

> ::: testing/mochitest/runtestsb2g.py
> @@ -213,5 @@
> > -        if (options.pidFile != ""):
> > -            f = open(options.pidFile + ".xpcshell.pid", 'w')
> > -            f.write("%s" % self.server._process.pid)
> > -            f.close()
> > -        self.server.ensureReady(90)
> 
> does startServers handle the pidfile stuff? I don't see it in my copy on m-c.

It does.  That's in the other patch.  I figured that having the pidfile stuff in the main file was harmless.  Desktop runs can have a pidfile now, which is unnecessary, but it reduces duplication between runtestsremote and runtestsb2g.

Comment 43

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-907770-Making-ssltunnel-run-for-android-and-b2g-.patch

Carrying r+ from jmaher.  Squashing commits; removing duplicated code.

Comment 44

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-907770-Test-changes-for-https-support-on-b2g-and.patch

Test case changes.  Squashing commits; carrying r+ from jmaher.

Comment 45

[Martin Thomson [:mt:]]

I had to rebase to get some changes.  I'll need to get a quick re-review as a result.  New patches coming.

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

Comment 46

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-907770-Making-ssltunnel-run-for-android-and-b2g-.patch

This required some surgery to deal with a merge conflict.  Probably worth another look.

Comment 47

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8400753 [details] [diff] [review]
0001-Bug-907770-Making-ssltunnel-run-for-android-and-b2g-.patch

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

this looks great.  thanks for the good comments and refactoring.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 49

[Martin Thomson [:mt:]]

So this is strange.  Rebasing wasn't required for me, but the push-to-hg command failed.  This is strange, because hg qimport was happy.

I'm not in the mood to search for root causes.  I've run a try build just in case.  I'll attach updated patches based on inbound tip as of a few minutes ago and then we can re-spin, assuming that try is happy.

https://tbpl.mozilla.org/?tree=Try&rev=2b5bb0340054

Comment 50

[Martin Thomson [:mt:]]

Attachment: 1-_PATCH_1_2__Bug_907770___Making_ssltunnel_run_for_android_and_b2g.patch

Rebased to inbound tip.  r+ from jmaher

Comment 51

[Martin Thomson [:mt:]]

Attachment: 2-_PATCH_2_2__Bug_907770___Test_changes_for_https_support_on.patch

r+ from jmaher

Comment 52

[Joel Maher ( :jmaher ) (UTC -8)]

we have a r+ here, are we waiting on anything?

Comment 54

[Martin Thomson [:mt:]]

We've been waiting on Bug 987406, which I see should be ready.  Now I have to rebase...again.

Comment 55

[Martin Thomson [:mt:]]

I probably made a hash of this, but the diff looks OK.  Let's see: https://tbpl.mozilla.org/?tree=Try&rev=ec37aba56d96

Comment 56

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-907770-Making-ssltunnel-run-for-android-and-b2g-.patch r=jmaher

Comment 57

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-907770-Test-changes-for-https-support-on-b2g-and.patch r=jmaher

Comment 58

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-907770-Making-ssltunnel-run-for-android-and-b2g-.patch r=jmaher

Finally ready.  r=jmaher

Comment 59

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-907770-Test-changes-for-https-support-on-b2g-and.patch

r=jmaher

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6a0f776c32
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9582a409beb

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c6a0f776c32
https://hg.mozilla.org/mozilla-central/rev/b9582a409beb

Comment 62

[Marco Castelluccio [:marco]]

I'm still experiencing an issue on b2g. I have a test that loads a HTTPS page in an iframe, its certificate is detected as invalid (bug 989806).

Comment 63

[Marco Castelluccio [:marco]]

The certificate is intermittently detected as invalid on Android too.