Closed Bug 1320643 Opened 8 years ago Closed 8 years ago

Marionette no longer loads pages served via wptserve through 127.0.0.1 but the public IP of the host

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: whimboo, Assigned: ato)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Bug 1320643 - Remove Device.setup_port_forward from mozrunner;
58 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1320643 - Let HTTPDs bind to custom interface;
58 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1320643 - Bind fixture servers to public interface when testing Fennec;
58 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1320643 - Use device manager directly when forwarding Marionette port;
58 bytes, text/x-review-board-request
whimboo
: review+
Details 
Clearly a fallout from bug 1103196. We should definitely not use the public IP but localhost (127.0.0.1) for serving test pages via wptserve. 

Another reason for this is that all test suites should actually cause a Firefox crash if network access happens. And that is triggered by non localhost pages.

Another side effect is that the Firewall is raising dialogs to allow/deny such connections.
Flags: needinfo?(ato)
Blocks: 1320642
Why should we “definitely not” do that?  We have to under some circumstances, but there should be no harm in binding to a public interface anyway because it simplifies the code path.

It should not cause a network access assertion crash because the public device on the system maps to the loopback device (see /etc/hosts).
Flags: needinfo?(ato)
It's not how the distinction works right now. We simply don't have bug 1272255 implemented yet for Marionette, so you would see it. By adding 'MOZ_DISABLE_NONLOCAL_CONNECTIONS': '1' to geckoinstance.py you will see that Firefox crashes right after startup.
Blocks: 1320073
Blocks: 1272255
I’m able to reproduce this with MOZ_DISABLE_NONLOCAL_CONNECTIONS=1, but we can’t add to the environment for as long as Fennec builds rely on binding to public interfaces to access the emulator (exactly why this is I don’t know, as one should think adb would be able to forward a port to it).
I don't think that is true given that we have Mochitests also running on Fennec and I believe this option should also be enabled for those. Tell me if I'm wrong.

What's more annoying now are really all the additional Firewall dialogs popping up for each run.

So why have you actually changed that behavior? Was it necessary to get TLS working?
(In reply to Henrik Skupin (:whimboo) from comment #4)
> I don't think that is true given that we have Mochitests also running on
> Fennec and I believe this option should also be enabled for those. Tell me
> if I'm wrong.

No, that was also my experience working with B2G which used the Android emulator.  `adb forward` was able to take a port on the loopback and pass all traffic through to the emulator.  The emulated device did not have to have to use a bridged adapter.

But I don’t know our Android emulator setup on try very well.  And by that I mean, “at all”.

> What's more annoying now are really all the additional Firewall dialogs
> popping up for each run.

I don’t understand what this is about.

> So why have you actually changed that behavior? Was it necessary to get TLS
> working?

Fennec requires the HTTP servers to bind to a public interface.  In the interest of simplifying the code, I did this for desktop too.  There’s nothing bad about this per se, other than the fact that the public IP isn’t routed to the loopback.
I think we should really revert this change and discuss any kind of simplification in a follow-up bug.
I disagree about reverting the patches on the grounds that Tier-1 try is green and Tier-3 Fennec has not regressed, and that it is an important milestone in Marionette to add TLS certificate management.  These changes have taken a long time to land after a protracted review, so I’m anxious to go through that again.

This fallout can be addressed by a follow-up.  Although it would be messy, possibly by reinstating the old behaviour that checks on `need_external_ip` if the appName is Fennec.  Alternatively finding out why Fennec needs the HTTPDs to bind on a public IP in the first place.  I suspect maybe it is because we need to adb port-forward the HTTPD’s ports also, and rewrite `marionette.absolute_url` to return a loopback device.
(In reply to Andreas Tolfsen ‹:ato› from comment #7)
> I disagree about reverting the patches on the grounds that Tier-1 try is
> green and Tier-3 Fennec has not regressed, and that it is an important
> milestone in Marionette to add TLS certificate management.  These changes

Keep in mind that Nightly and Release builds might have been a different set of configs. So while you are not seeing it for CI builds you might see for others (branded builds).

> have taken a long time to land after a protracted review, so I’m anxious to
> go through that again.

Sure, and no worries. And I understand that. We should at least figure out the remaining issues quickly and get those fixed before you ask for uplift.
Comment on attachment 8814975 [details]
Bug 1320643 - Reverse forward HTTPD ports to host on emulator;

https://reviewboard.mozilla.org/r/96018/#review96528

::: testing/marionette/client/marionette_driver/geckoinstance.py:346
(Diff revision 2)
>          self.runner.device.start_logcat(**logcat_args)
> +
> +        # forward marionette and httpd ports to emulator
>          self.runner.device.setup_port_forwarding(
> -            local_port=self.marionette_port,
> -            remote_port=self.marionette_port,
> +            local_port=self.marionette_port, remote_port=self.marionette_port)
> +        for _, (url, _) in self.runner.fixture_servers.iteritems():

This line will break starting Marionette because there is no fixture_servers attribute on the runner.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment on attachment 8814975 [details]
Bug 1320643 - Reverse forward HTTPD ports to host on emulator;

https://reviewboard.mozilla.org/r/96018/#review96846

::: testing/marionette/client/marionette_driver/geckoinstance.py:9
(Diff revision 3)
>  
>  import os
>  import sys
>  import tempfile
>  import time
> +import urlparse

not needed right?

::: testing/marionette/harness/marionette/runner/base.py:914
(Diff revision 3)
> +        if self.emulator:
> +            for url in iter_url(servers):
> +                port = urlparse.urlparse(url).port
> +                self.logger.info("Forwarding port %d to emulator" % port)
> +                self.marionette.instance.runner.device.setup_port_forwarding(
> +                    local_port=port, remote_port=port)

I like that! Kinda elegant solution now.
Attachment #8814975 - Flags: review?(hskupin) → review+
Comment on attachment 8814975 [details]
Bug 1320643 - Reverse forward HTTPD ports to host on emulator;

https://reviewboard.mozilla.org/r/96018/#review96848

Looks like this actually completely breaks tests on Android:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7d6e32e4940&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=32095177
Attachment #8814975 - Flags: review+ → review-
Comment on attachment 8814974 [details]
Bug 1320643 - Remove Device.setup_port_forward from mozrunner;

https://reviewboard.mozilla.org/r/96016/#review96852

Desktop tests are passing and correctly use localhost again. So I don't see a problem in this patch, but please check what's up with the Android bustage.
Comment on attachment 8814974 [details]
Bug 1320643 - Remove Device.setup_port_forward from mozrunner;

https://reviewboard.mozilla.org/r/96016/#review96854
Attachment #8814974 - Flags: review?(hskupin) → review+
(In reply to Henrik Skupin (:whimboo) from comment #17)
> Desktop tests are passing and correctly use localhost again. So I don't see
> a problem in this patch, but please check what's up with the Android bustage.

The traceback from the Android emulator:

Traceback (most recent call last):
  File "/home/worker/workspace/build/tests/marionette/marionette/runtests.py", line 87, in cli
    failed = harness_instance.run()
  File "/home/worker/workspace/build/tests/marionette/marionette/runtests.py", line 69, in run
    runner.run_tests(tests)
  File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette/runner/base.py", line 814, in run_tests
    self.fixture_servers = self.start_fixture_servers()
  File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette/runner/base.py", line 914, in start_fixture_servers
    local_port=port, remote_port=port)
  File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/mozrunner/devices/base.py", line 205, in setup_port_forwarding
    self.dm.forward('tcp:%d' % int(local_port), 'tcp:%d' % int(remote_port))
  File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/mozdevice/devicemanagerADB.py", line 186, in forward
    raise DMError("Failed to forward socket connection.")
DMError: Failed to forward socket connection.

I have no idea what causes this yet…
Did you try to run the above multiple times? Maybe we also have to call remove_forward() when we are done? Could be that the former forwarded ports are still in effect for you?
I tried to get tests working on Android but was not successful. As it looks like it fails to correctly setup  the profile on my machine and as such the socket listener does not start. As of now I might not be of help regarding this issue. But please check my last comment. Maybe you run it multiple times and forward can only be called once for the same port?
Maybe Geoff could be of help here. Please see comment 19. Thanks.
Flags: needinfo?(gbrown)
Attachment #8814975 - Flags: review?(hskupin)
I was eventually able to find this in marionette_raw.log:

{"source": "Marionette-based Tests", "thread": "MainThread", "time": 1480695173533, "action": "log", "message": "Starting fixture servers", "level": "INFO", "pid": 1140}
{"source": "Marionette-based Tests", "thread": "MainThread", "time": 1480695173548, "action": "log", "message": "Fixture server listening on http://127.0.0.1:33319/", "level": "INFO", "pid": 1140}
{"source": "Marionette-based Tests", "thread": "MainThread", "time": 1480695173548, "action": "log", "message": "Fixture server listening on https://127.0.0.1:35970/", "level": "INFO", "pid": 1140}
{"source": "Marionette-based Tests", "thread": "MainThread", "time": 1480695173549, "action": "log", "message": "Forwarding port 33319 to emulator", "level": "INFO", "pid": 1140}
{"thread": "MainThread", "level": "DEBUG", "pid": 1140, "component": "mozdevice", "source": "Marionette-based Tests", "time": 1480695173549, "action": "log", "message": "_checkCmd - command: /home/worker/workspace/build/android-sdk18/platform-tools/adb -s emulator-5554 forward tcp:33319 tcp:33319"}
{"thread": "ProcessReader", "level": "DEBUG", "pid": 1140, "component": "mozdevice", "source": "Marionette-based Tests", "time": 1480695173556, "action": "log", "message": "error: cannot bind to socket"}
As we figured out with Geoff's help on IRC, we may need reverse instead of forward because Fennec in the emulator or the device has to reach the httpds on the local machine.
Flags: needinfo?(gbrown)
Attachment #8814975 - Attachment is obsolete: true
Comment on attachment 8814974 [details]
Bug 1320643 - Remove Device.setup_port_forward from mozrunner;

https://reviewboard.mozilla.org/r/96016/#review97570

this is great- there is one instance of this in  the tree which you have up for review to remove it, so I am sold!
Attachment #8814974 - Flags: review?(jmaher) → review+
Comment on attachment 8816495 [details]
Bug 1320643 - Bind fixture servers to public interface when testing Fennec;

https://reviewboard.mozilla.org/r/97206/#review97674

::: testing/marionette/harness/marionette/runner/base.py:904
(Diff revision 3)
>              for failed_test in self.failures:
>                  self.logger.info('{}'.format(failed_test[0]))
>  
>      def start_fixture_servers(self):
>          root = self.server_root or os.path.join(os.path.dirname(here), "www")
> +        if self.emulator:

This would only make use of moznetwork.get_ip() for tests running in an emulator, but not on a real device. Please make sure to use this code path for Fennec builds as we had it before the TLS patch.
Attachment #8816495 - Flags: review?(hskupin) → review-
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97676

::: testing/marionette/harness/marionette/runner/serve.py:138
(Diff revision 2)
>          if self.proc is not None:
>              return self.proc.is_alive()
>          return False
>  
>  
> -def http_server(doc_root, ssl_config, **kwargs):
> +def http_server(doc_root, ssl_config, host="127.0.0.1", **kwargs):

While we are at this line why do we need ssl_config as parameter? This is never used for HTTP servers.

::: testing/marionette/harness/marionette/runner/serve.py:139
(Diff revision 2)
>              return self.proc.is_alive()
>          return False
>  
>  
> -def http_server(doc_root, ssl_config, **kwargs):
> -    return httpd.FixtureServer(doc_root, url="http://%s:0/" % moznetwork.get_ip())
> +def http_server(doc_root, ssl_config, host="127.0.0.1", **kwargs):
> +    return httpd.FixtureServer(doc_root, url="http://%s:0/" % host, **kwargs)

As mentionend already for the TLS patch, please make use of .format(). We no longer want to introduce more %s formatted lines. Thanks.
Attachment #8816494 - Flags: review?(hskupin) → review-
Comment on attachment 8816496 [details]
Bug 1320643 - Use device manager directly when forwarding Marionette port;

https://reviewboard.mozilla.org/r/97208/#review97678
Attachment #8816496 - Flags: review?(hskupin) → review+
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97676

> While we are at this line why do we need ssl_config as parameter? This is never used for HTTP servers.

Because the `http_server` and `https_server` functions need to take the same input because they are listed in `registered_servers` and iterated over and called generically.

> As mentionend already for the TLS patch, please make use of .format(). We no longer want to introduce more %s formatted lines. Thanks.

I don’t understand why not.  I feel these issues are holding this fix back, and we need to have it ready for Aurora for it to make ESR, and try is green.
Comment on attachment 8816495 [details]
Bug 1320643 - Bind fixture servers to public interface when testing Fennec;

https://reviewboard.mozilla.org/r/97206/#review97674

> This would only make use of moznetwork.get_ip() for tests running in an emulator, but not on a real device. Please make sure to use this code path for Fennec builds as we had it before the TLS patch.

Fixed.  It looks like the original check was just `self.appInfo == "fennec"`: https://github.com/mozilla/gecko-dev/blob/2b0c1a59c5869f3391dfcb99e684390b6eaa7d1f/testing/marionette/harness/marionette/runner/base.py#L777
Comment on attachment 8816495 [details]
Bug 1320643 - Bind fixture servers to public interface when testing Fennec;

https://reviewboard.mozilla.org/r/97206/#review97780
Attachment #8816495 - Flags: review?(hskupin) → review+
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97676

> Because the `http_server` and `https_server` functions need to take the same input because they are listed in `registered_servers` and iterated over and called generically.

It's just odd to see SSL config settings for HTTP.

> I don’t understand why not.  I feel these issues are holding this fix back, and we need to have it ready for Aurora for it to make ESR, and try is green.

format() is way better in handling various types. We did a mass-refactor to get rid of %s not that long ago. So seeing new %s formatting introduced, adds inconsistency again. Seeing how strict you are for Marionette server I wonder about your reaction now. :) But go ahead and land those patches, and please make sure to use format() in the future.
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97784
Attachment #8816494 - Flags: review?(hskupin) → review+
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97676

> format() is way better in handling various types. We did a mass-refactor to get rid of %s not that long ago. So seeing new %s formatting introduced, adds inconsistency again. Seeing how strict you are for Marionette server I wonder about your reaction now. :) But go ahead and land those patches, and please make sure to use format() in the future.

OK.  Why do we want to limit certain features of the language?  If we want to enforce this consistently we ought to point it out when running the linter.  (I have a patch in progress to enable eslint for the JS code in Marionette as well.)

I’ll address this now if `format` is used consistently.
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97676

> It's just odd to see SSL config settings for HTTP.

I don’t think this is an unknown programming pattern.  If Python supported discarding arguments, it would perhaps be easier to read.
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97676

> I don’t think this is an unknown programming pattern.  If Python supported discarding arguments, it would perhaps be easier to read.

You could add `*args` before `**kwargs` to make it at least less odd to see the config argument in the http_server() function, while still passing it in by the caller.
Comment on attachment 8816495 [details]
Bug 1320643 - Bind fixture servers to public interface when testing Fennec;

https://reviewboard.mozilla.org/r/97206/#review97862

::: testing/marionette/harness/marionette/runner/serve.py:139
(Diff revision 5)
>              return self.proc.is_alive()
>          return False
>  
>  
>  def http_server(doc_root, ssl_config, host="127.0.0.1", **kwargs):
> -    return httpd.FixtureServer(doc_root, url="http://%s:0/" % host, **kwargs)
> +    return httpd.FixtureServer(doc_root, url="http://{}s:0/".format(host), **kwargs)

You missed to remove the `s`.
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97676

> You could add `*args` before `**kwargs` to make it at least less odd to see the config argument in the http_server() function, while still passing it in by the caller.

The reason I don’t pop it off `kwargs` or give it a default value is because it is a mandatory argument, and that it’s clear from reading the function signature that you need to pass it at least two arguments.  I feel changing it will make it less clear how to use the function, but since I don’t have any particularly strong feelings about it, I can address it in a more subtle way.
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97676

> The reason I don’t pop it off `kwargs` or give it a default value is because it is a mandatory argument, and that it’s clear from reading the function signature that you need to pass it at least two arguments.  I feel changing it will make it less clear how to use the function, but since I don’t have any particularly strong feelings about it, I can address it in a more subtle way.

Just leave it as it is right now. It's fine.
Comment on attachment 8816494 [details]
Bug 1320643 - Let HTTPDs bind to custom interface;

https://reviewboard.mozilla.org/r/97204/#review97676

> Just leave it as it is right now. It's fine.

Okay, I tried to address this but it caused a bunch of different problems down the line.  I’ve now reverted the fix that doesn’t expose `ssl_config` as the second argument.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29e885d1bf97
Use device manager directly when forwarding Marionette port; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/d4657cdb0812
Remove Device.setup_port_forward from mozrunner; r=jmaher,whimboo
https://hg.mozilla.org/integration/autoland/rev/d237751e0dea
Let HTTPDs bind to custom interface; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/dc7d74a8a4ba
Bind fixture servers to public interface when testing Fennec; r=whimboo
Sheriffs: Test-only uplift.
Whiteboard: [checkin-needed-aurora]
Uplift requests should be set when the code has been merged to m-c, and proven that no other regressions have been introduced. Resetting request for now.
Whiteboard: [checkin-needed-aurora]
I requested a new Nightly build with this change included. We will be able to test this in a couple of hours and if all goes well, we can uplift to aurora.
Tests per commit on autoland and mozilla-central look fine. Same applies to our Nightly build tests in Mozmill CI. The regression has been fixed.

Please uplift this test-only patch to mozilla-aurora. Thanks.
Whiteboard: [checkin-needed-aurora]
Whiteboard: [checkin-needed-aurora]
Depends on: 1323387
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: