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

RESOLVED FIXED in mozilla31

Status

Testing
Mochitest
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jchen, Assigned: mt)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla31
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 19 obsolete attachments)

41.28 KB, patch
Details | Diff | Splinter Review
2.79 KB, patch
Details | Diff | Splinter Review
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.
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.
Blocks: 908912
Blocks: 904257
Blocks: 901356
content/base/test/test_CrossSiteXHR_origin.html also suffers from this bug, I think.
And dom/permission/tests/test_permission_basics.html
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
Summary: HTTPS hosts don't work in Mochitests on Android → HTTPS hosts don't work in Mochitests on Android/B2G
this is a known issue and somebody just needs to spend a day figuring it out and testing it.
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.
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.
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.
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.
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.
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.
What is the path forward here?  Not testing any SSL stuff on b2g is ... unfortunate.

Updated

4 years ago
See Also: → bug 975149
(Assignee)

Updated

4 years ago
Duplicate of this bug: 975149
(Assignee)

Comment 14

4 years ago
Created attachment 8381045 [details] [diff] [review]
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).
(Assignee)

Comment 15

4 years ago
OK, now I'm sad.  The above works for content/base/test/test_CrossSiteXHR.html
Assignee: nobody → martin.thomson
I have a feeling bug 676972 has the same root cause or at least is very similar.
See Also: → bug 676972
(Assignee)

Comment 17

4 years ago
Created attachment 8383422 [details] [diff] [review]
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.
Attachment #8381045 - Attachment is obsolete: true
(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 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.
(Assignee)

Comment 20

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 942367
(Assignee)

Comment 21

4 years ago
Created attachment 8383963 [details] [diff] [review]
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.
Attachment #8383422 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 8383964 [details] [diff] [review]
0002-Bug-907770-Enabling-cross-origin-tests-for-Android.patch

Cross origin tests in content/base enabled.  WFM.
(Reporter)

Comment 23

4 years ago
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
Are these patches ready to be reviewed?
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 25

4 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 26

4 years ago
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.
(Assignee)

Comment 27

4 years ago
Created attachment 8390716 [details] [diff] [review]
Making ssltunnel run for mochitest-remote
(Assignee)

Comment 28

4 years ago
Created attachment 8390717 [details] [diff] [review]
Enabling cross origin tests for Android.
(Assignee)

Comment 29

4 years ago
Created attachment 8390718 [details] [diff] [review]
Enabling dom/permission/tests/test_permission_basics.html
(Assignee)

Comment 30

4 years ago
Created attachment 8390719 [details] [diff] [review]
Restoring https test.
(Assignee)

Updated

4 years ago
Attachment #8383964 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8383963 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8390716 - Flags: review?(jmaher)
(Assignee)

Updated

4 years ago
Attachment #8390717 - Flags: review?(jmaher)
(Assignee)

Updated

4 years ago
Attachment #8390718 - Flags: review?(jmaher)
(Assignee)

Updated

4 years ago
Attachment #8390719 - Flags: review?(jmaher)
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.
Attachment #8390716 - Flags: review?(jmaher) → review+
Attachment #8390717 - Flags: review?(jmaher) → review+
Attachment #8390718 - Flags: review?(jmaher) → review+
Comment on attachment 8390719 [details] [diff] [review]
Restoring https test.

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

thanks!
Attachment #8390719 - Flags: review?(jmaher) → review+
(Assignee)

Comment 33

4 years ago
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
(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?
(Assignee)

Comment 35

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 987406
(Assignee)

Comment 36

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

This test needs further work.
Attachment #8390718 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
Created attachment 8397231 [details] [diff] [review]
0001-Bug-907770-Making-ssltunnel-run-for-mochitest-remote.patch

Carrying forward r+ from jmaher.  Unbitrot, review comments.
Attachment #8390716 - Attachment is obsolete: true
Attachment #8397231 - Flags: review?
(Assignee)

Comment 38

4 years ago
Created attachment 8397232 [details] [diff] [review]
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.
Attachment #8397232 - Flags: review?(jmaher)
(Assignee)

Comment 39

4 years ago
Created attachment 8397235 [details] [diff] [review]
0003-Bug-907770-Enabling-cross-origin-tests-for-Android.patch

Carrying r+ from jmaher.
Attachment #8390717 - Attachment is obsolete: true
Attachment #8397235 - Flags: review+
(Assignee)

Comment 40

4 years ago
Created attachment 8397236 [details] [diff] [review]
0004-Bug-907770-Restoring-https-test.patch

Carrying r+ from jmaher.
Attachment #8397236 - Flags: review+
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.
Attachment #8397232 - Flags: review?(jmaher) → review+
(Assignee)

Comment 42

4 years ago
(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.
(Assignee)

Comment 43

4 years ago
Created attachment 8398006 [details] [diff] [review]
0001-Bug-907770-Making-ssltunnel-run-for-android-and-b2g-.patch

Carrying r+ from jmaher.  Squashing commits; removing duplicated code.
Attachment #8397231 - Attachment is obsolete: true
Attachment #8397232 - Attachment is obsolete: true
Attachment #8397231 - Flags: review?
Attachment #8398006 - Flags: review+
(Assignee)

Comment 44

4 years ago
Created attachment 8398007 [details] [diff] [review]
0002-Bug-907770-Test-changes-for-https-support-on-b2g-and.patch

Test case changes.  Squashing commits; carrying r+ from jmaher.
Attachment #8390719 - Attachment is obsolete: true
Attachment #8397235 - Attachment is obsolete: true
Attachment #8397236 - Attachment is obsolete: true
Attachment #8398007 - Flags: review+
(Assignee)

Comment 45

4 years ago
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
(Assignee)

Comment 46

4 years ago
Created attachment 8400753 [details] [diff] [review]
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.
Attachment #8398006 - Attachment is obsolete: true
Attachment #8400753 - Flags: review?(jmaher)
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.
Attachment #8400753 - Flags: review?(jmaher) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
(Assignee)

Comment 49

4 years ago
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
(Assignee)

Comment 50

4 years ago
Created attachment 8403655 [details] [diff] [review]
1-_PATCH_1_2__Bug_907770___Making_ssltunnel_run_for_android_and_b2g.patch

Rebased to inbound tip.  r+ from jmaher
Attachment #8400753 - Attachment is obsolete: true
Attachment #8403655 - Flags: review+
(Assignee)

Comment 51

4 years ago
Created attachment 8403656 [details] [diff] [review]
2-_PATCH_2_2__Bug_907770___Test_changes_for_https_support_on.patch

r+ from jmaher
Attachment #8398007 - Attachment is obsolete: true
Attachment #8403656 - Flags: review+
Depends on: 994936
(Assignee)

Updated

4 years ago
Blocks: 995328
(Assignee)

Updated

4 years ago
No longer blocks: 942367
we have a r+ here, are we waiting on anything?
Duplicate of this bug: 907979
(Assignee)

Comment 54

4 years ago
We've been waiting on Bug 987406, which I see should be ready.  Now I have to rebase...again.
(Assignee)

Comment 55

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

Comment 56

4 years ago
Created attachment 8407881 [details] [diff] [review]
0001-Bug-907770-Making-ssltunnel-run-for-android-and-b2g-.patch r=jmaher
Attachment #8403655 - Attachment is obsolete: true
Attachment #8407881 - Flags: review+
(Assignee)

Comment 57

4 years ago
Created attachment 8407882 [details] [diff] [review]
0002-Bug-907770-Test-changes-for-https-support-on-b2g-and.patch r=jmaher
Attachment #8403656 - Attachment is obsolete: true
Attachment #8407882 - Flags: review+
(Assignee)

Comment 58

4 years ago
Created attachment 8410322 [details] [diff] [review]
0001-Bug-907770-Making-ssltunnel-run-for-android-and-b2g-.patch r=jmaher

Finally ready.  r=jmaher
Attachment #8407881 - Attachment is obsolete: true
Attachment #8410322 - Flags: review+
(Assignee)

Comment 59

4 years ago
Created attachment 8410323 [details] [diff] [review]
0002-Bug-907770-Test-changes-for-https-support-on-b2g-and.patch

r=jmaher
Attachment #8407882 - Attachment is obsolete: true
Attachment #8410323 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c6a0f776c32
https://hg.mozilla.org/mozilla-central/rev/b9582a409beb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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).
The certificate is intermittently detected as invalid on Android too.

Updated

4 years ago
See Also: → bug 1080566
You need to log in before you can comment on or make changes to this bug.