Closed
Bug 907770
Opened 11 years ago
Closed 10 years ago
HTTPS hosts don't work in Mochitests on Android/B2G
Categories
(Testing :: Mochitest, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla31
People
(Reporter: jchen, Assigned: mt)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 19 obsolete files)
41.28 KB,
patch
|
mt
:
review+
|
Details | Diff | Splinter Review |
2.79 KB,
patch
|
mt
:
review+
|
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.
Comment 1•11 years ago
|
||
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•11 years ago
|
||
content/base/test/test_CrossSiteXHR_origin.html also suffers from this bug, I think.
Comment 3•11 years ago
|
||
And dom/permission/tests/test_permission_basics.html
Comment 4•11 years ago
|
||
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
Comment 5•11 years ago
|
||
this is a known issue and somebody just needs to spend a day figuring it out and testing it.
Comment 6•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
(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•10 years ago
|
||
OK, now I'm sad. The above works for content/base/test/test_CrossSiteXHR.html
Updated•10 years ago
|
Assignee: nobody → martin.thomson
I have a feeling bug 676972 has the same root cause or at least is very similar.
See Also: → 676972
Assignee | ||
Comment 17•10 years ago
|
||
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
Comment 18•10 years ago
|
||
(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•10 years ago
|
||
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•10 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 | ||
Comment 21•10 years ago
|
||
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•10 years ago
|
||
Cross origin tests in content/base enabled. WFM.
Reporter | ||
Comment 23•10 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•10 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•10 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•10 years ago
|
||
Assignee | ||
Comment 28•10 years ago
|
||
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Comment 30•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8383964 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8383963 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8390716 -
Flags: review?(jmaher)
Assignee | ||
Updated•10 years ago
|
Attachment #8390717 -
Flags: review?(jmaher)
Assignee | ||
Updated•10 years ago
|
Attachment #8390718 -
Flags: review?(jmaher)
Assignee | ||
Updated•10 years ago
|
Attachment #8390719 -
Flags: review?(jmaher)
Comment 31•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8390717 -
Flags: review?(jmaher) → review+
Updated•10 years ago
|
Attachment #8390718 -
Flags: review?(jmaher) → review+
Comment 32•10 years ago
|
||
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•10 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
Comment 34•10 years ago
|
||
(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•10 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 | ||
Comment 36•10 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•10 years ago
|
||
Carrying forward r+ from jmaher. Unbitrot, review comments.
Attachment #8390716 -
Attachment is obsolete: true
Attachment #8397231 -
Flags: review?
Assignee | ||
Comment 38•10 years ago
|
||
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•10 years ago
|
||
Carrying r+ from jmaher.
Attachment #8390717 -
Attachment is obsolete: true
Attachment #8397235 -
Flags: review+
Comment 41•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 years ago
|
||
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•10 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•10 years ago
|
||
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 47•10 years ago
|
||
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•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 49•10 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•10 years ago
|
||
Rebased to inbound tip. r+ from jmaher
Attachment #8400753 -
Attachment is obsolete: true
Attachment #8403655 -
Flags: review+
Assignee | ||
Comment 51•10 years ago
|
||
r+ from jmaher
Attachment #8398007 -
Attachment is obsolete: true
Attachment #8403656 -
Flags: review+
Comment 52•10 years ago
|
||
we have a r+ here, are we waiting on anything?
Assignee | ||
Comment 54•10 years ago
|
||
We've been waiting on Bug 987406, which I see should be ready. Now I have to rebase...again.
Assignee | ||
Comment 55•10 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•10 years ago
|
||
Attachment #8403655 -
Attachment is obsolete: true
Attachment #8407881 -
Flags: review+
Assignee | ||
Comment 57•10 years ago
|
||
Attachment #8403656 -
Attachment is obsolete: true
Attachment #8407882 -
Flags: review+
Assignee | ||
Comment 58•10 years ago
|
||
Finally ready. r=jmaher
Attachment #8407881 -
Attachment is obsolete: true
Attachment #8410322 -
Flags: review+
Assignee | ||
Comment 59•10 years ago
|
||
r=jmaher
Attachment #8407882 -
Attachment is obsolete: true
Attachment #8410323 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 60•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6a0f776c32 https://hg.mozilla.org/integration/mozilla-inbound/rev/b9582a409beb
Flags: in-testsuite+
Keywords: checkin-needed
Comment 61•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4c6a0f776c32 https://hg.mozilla.org/mozilla-central/rev/b9582a409beb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 62•10 years ago
|
||
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•10 years ago
|
||
The certificate is intermittently detected as invalid on Android too.
You need to log in
before you can comment on or make changes to this bug.
Description
•