Closed Bug 882932 Opened 6 years ago Closed 6 years ago

Remote mochitests should invoke the httpd.js from hostutils, not from the changeset

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(firefox24 fixed)

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- fixed

People

(Reporter: bholley, Assigned: gbrown)

References

Details

Attachments

(2 files, 3 obsolete files)

In bug 880917, I pushed a joint change to httpd.js and Gecko for the evaluation of SJS files. These changes are interdependent - SJS files will fail to evaluate if either of httpd.js or Gecko changes without the other.

This went orange on Android and B2G when I pushed to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=71c1ce2cb0a4

Clint and I investigated. As it turns out, the server invocation ends up using the xpcshell (and thus Gecko) from the hostutils bundle, but the httpd.js from test.zip.

This is the relevant line:

args: ['/builds/tegra-261/test/build/hostutils/bin/xpcshell', '-g', '/builds/tegra-261/test/build/hostutils/xre', '-v', '170', '-f', './httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpwEOzpa';const _SERVER_PORT = '30261'; const _SERVER_ADDR = '10.250.49.167';\n                     const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', './server.js']

The simplest path to victory here is to just to use the httpd.js from hostutils as well, so that they match. When the hostutils bundle gets updated, it will pick up both of my changes together.

This blocks a large landing that blocks another large landing, so I'd appreciate if we could get this fixed asap. :-)
This seems really fragile either way. Either changes to httpd.js in the source can break running it in a non-matching xpcshell (which is what you hit), or changes to sjs files could break running in a non-matching xpcshell+httpd.js.

This is part of the reason we'd like to move away from using httpd.js. :-/
this is the first time since 2009 that we have ran into an issue where we needed to update hostutils.  I recall updating it a while back for another unrelated reason.  We could just manifest out all the .sjs files from mochitests and not run them on mobile, then we could avoid this altogether.
Can we just do the most expedient thing for now, whatever it is? I'd really like to get the dependency chain landed (among other things, it fixes some bad crashes).

I could also add some sniffing into httpd.js as a workaround, but I'd really rather avoid that.
The quickest thing by far for now is to update hostutils.zip. Gbrown, can you drive that with kmoir/aki? I think one of them is the right person from releng for this.

I know this set up is fragile and that is one of many reasons we want to move away from httpd.js, but that's not going to happen right now. What's interesting is that we've had very few bustages like this (though that's more than been made up for by the amount of confusion and consternation that this design has caused).

Clint
Flags: needinfo?(gbrown)
(In reply to Clint Talbert ( :ctalbert ) from comment #4)
> The quickest thing by far for now is to update hostutils.zip. Gbrown, can
> you drive that with kmoir/aki? I think one of them is the right person from
> releng for this.

Do you mean just doing a new Gecko drop? How would that solve this problem?
:bholley- updating hostutils.zip would give us a fully up to date set of libraries and xpcshell.  That means that when we use httpd.js or other calls from the tests package, they will reference up to date code with the expected api set.
(In reply to Joel Maher (:jmaher) from comment #7)
> :bholley- updating hostutils.zip would give us a fully up to date set of
> libraries and xpcshell.  That means that when we use httpd.js or other calls
> from the tests package, they will reference up to date code with the
> expected api set.

But the problem here is that I'm landing mutually-interdependent changes to Gecko and httpd.js. They both have to change exactly at the same time. If one changes and the other doesn't, we break.

I could also just land a hack in httpd.js to check the IID of nsIXPCComponents_Utils against the one we ship in hostutils.zip. If there's not a great architectural solution on the automation sign then that's probably the simplest path to victory.
if that is the case, then we would need to ensure we have a httpd.js and server.js in the hostutils.zip and then modify the test harness to use that.  Oh, how I wish for a time in the future where we remove our .sjs dependency on httpd.js!
(In reply to Joel Maher (:jmaher) from comment #9)
> if that is the case, then we would need to ensure we have a httpd.js and
> server.js in the hostutils.zip and then modify the test harness to use that.
> Oh, how I wish for a time in the future where we remove our .sjs dependency
> on httpd.js!

Right, this was I meant this bug to be about. Is this something that's worthwhile infra-wise and doable on a today-or-tomorrow-ish timescale? Or should I do the hack from comment 8?
(In reply to Bobby Holley (:bholley) from comment #8)
> (In reply to Joel Maher (:jmaher) from comment #7)
> > :bholley- updating hostutils.zip would give us a fully up to date set of
> > libraries and xpcshell.  That means that when we use httpd.js or other calls
> > from the tests package, they will reference up to date code with the
> > expected api set.
> 
> But the problem here is that I'm landing mutually-interdependent changes to
> Gecko and httpd.js. They both have to change exactly at the same time. If
> one changes and the other doesn't, we break.

This will break, badly, the way things are now.

All trees/branches use the _same_ xpcshell in the same hostutils.

(In reply to Bobby Holley (:bholley) from comment #10)
> Right, this was I meant this bug to be about. Is this something that's
> worthwhile infra-wise and doable on a today-or-tomorrow-ish timescale? Or
> should I do the hack from comment 8?

I'd be shocked if we can even achieve this switch reliably in a single week, even if all of one human resource was devoted to it. Given the "all trees would have to change" aspect.
I found a httpd.js in the hostutils.zip:/bin/components directory.  I have hacked up mochitest (we would need reftest/xpcshell also) to have a --httpd-path cli flag.  Oh yeah, it gets ugly;)

On my try server patch, I have hardcoded that, maybe that is the best approach to get this in until we could update the buildbot-configs to work as expected.
(In reply to Justin Wood (:Callek) from comment #11)
> > But the problem here is that I'm landing mutually-interdependent changes to
> > Gecko and httpd.js. They both have to change exactly at the same time. If
> > one changes and the other doesn't, we break.
> 
> This will break, badly, the way things are now.
> 
> All trees/branches use the _same_ xpcshell in the same hostutils.

That's fine. As long as the httpd.js and xpcshell have the same gecko revision, there's no problem. It doesn't matter if the server's Gecko matches the client.

> I'd be shocked if we can even achieve this switch reliably in a single week,
> even if all of one human resource was devoted to it. Given the "all trees
> would have to change" aspect.

Would they? I'd think just the hostutils package would need to change.
(In reply to Bobby Holley (:bholley) from comment #14)
> (In reply to Justin Wood (:Callek) from comment #11)
> > I'd be shocked if we can even achieve this switch reliably in a single week,
> > even if all of one human resource was devoted to it. Given the "all trees
> > would have to change" aspect.
> 
> Would they? I'd think just the hostutils package would need to change.

Well we'd need to modify how we call things to be sure we use the hostutils httpd.js and related. sounds like joel found out this can be changed in-tree, and if thats true then it may be less time than I expected.
I'm going to experiment with a hack in parallel:

https://tbpl.mozilla.org/?tree=Try&rev=a1a6918f1956
my experiments seemed to work for mochitest (although a persistent error on mochitest-3 that seems related to when I pushed, not the patch).  I don't have it working for reftests, I will try again this morning.
(In reply to Clint Talbert ( :ctalbert ) from comment #4)
> Gbrown, can you drive that with kmoir/aki? 

:jmaher is running with this; I'm lending a hand as needed.
Flags: needinfo?(gbrown)
attaching this patch as it looks pretty good on try server (waiting on r1/r2 results before knowing 100%).  gbrown is going to take over making sure this is a good solution for local developers.
This seems the most direct route to working in local runs:
 - use the hard-coded default httpd-path if not specified
 - specify the old httpd-path in makefile targets

I tested make targets mochitest, reftest, mochitest-remote, mochitest-robocop, mochitest-robotium, and reftest-remote.

Do we need to modify b2g at all in this bug?

New try run: https://tbpl.mozilla.org/?tree=Try&rev=b8e37f1b78a0
Attachment #764405 - Flags: feedback?(jmaher)
Comment on attachment 764405 [details] [diff] [review]
tweak jmaher's patch to work locally

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

this is looking good in general, green on try, works for developers, I would just like the comments cleaned up instead of "JMAHER - let the hacking begin" :)

::: layout/tools/reftest/remotereftest.py
@@ +156,5 @@
>              f.close()
>  
>          #JMAHER - let the hacking begin
> +        if not options.httpdPath:
> +            options.httpdPath = os.path.join(options.utilityPath, "components")

a better comment indicating the hack would be appreciated :)
Attachment #764405 - Flags: feedback?(jmaher) → feedback+
Combines our patches, with comments cleaned up.

One more try: https://tbpl.mozilla.org/?tree=Try&rev=130d46ca756d
Attachment #764342 - Attachment is obsolete: true
Attachment #764405 - Attachment is obsolete: true
Attachment #764774 - Flags: review?(jmaher)
Comment on attachment 764774 [details] [diff] [review]
use httpd.js from android hostutils.zip package instead of tests.zip

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

on a try server run with this, we need to pop open the full log and verify the httpd.js path :)

::: testing/mochitest/runtests.py
@@ +315,5 @@
>      options.webServer = self._automation.DEFAULT_WEB_SERVER
>      options.httpPort = self._automation.DEFAULT_HTTP_PORT
>      options.sslPort = self._automation.DEFAULT_SSL_PORT
>      options.webSocketPort = self._automation.DEFAULT_WEBSOCKET_PORT
> +    options.httpdPath = '.'

I recall this being quirky, we might want to leave this line out since you have a testsuite-targets.mk fix.
Attachment #764774 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #23)
> ::: testing/mochitest/runtests.py
> @@ +315,5 @@
> >      options.webServer = self._automation.DEFAULT_WEB_SERVER
> >      options.httpPort = self._automation.DEFAULT_HTTP_PORT
> >      options.sslPort = self._automation.DEFAULT_SSL_PORT
> >      options.webSocketPort = self._automation.DEFAULT_WEBSOCKET_PORT
> > +    options.httpdPath = '.'
> 
> I recall this being quirky, we might want to leave this line out since you
> have a testsuite-targets.mk fix.

The testsuite-targets.mk fix only affects remote targets. For desktop, '.' seems to always work.
I am struggling with the b2g reftests. Currently these tests do not seem to use hostutils, instead invoking:

'/home/cltbld/talos-slave/test/build/xre/bin/xpcshell', '-g', '/home/cltbld/talos-slave/test/build/xre/bin', '-v', '170', '-f', '/home/cltbld/talos-slave/test/build/tests/reftest/reftest/components/httpd.js', ...

xre/bin appears to be populated from a zip downloaded from http://runtime-binaries.pvt.build.mozilla.org/. So this seems to have the same problem: xpcshell comes from a location that will likely be out of sync with httpd.js.

b2g mochitests are fine using:

'/builds/slave/test/build/xre/bin/xpcshell', '-g', '/builds/slave/test/build/xre/bin', '-v', '170', '-f', './httpd.js', ...

but if I use those paths for xpcshell and httpd.js during b2g reftests, httpd.js is not found:

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

I must be missing something...
ahal, can you help us understand how b2g reftests run the webserver (xpcshell + httpd.js) ?
Flags: needinfo?(ahalberstadt)
(In reply to Geoff Brown [:gbrown] from comment #25)
> xre/bin appears to be populated from a zip downloaded from
> http://runtime-binaries.pvt.build.mozilla.org/. So this seems to have the
> same problem: xpcshell comes from a location that will likely be out of sync
> with httpd.js.

Yes, this is correct. I didn't know hostutils was a thing, should b2g unittests just be using that? Is mozharness capable of accessing it?

> b2g mochitests are fine using:
> 
> '/builds/slave/test/build/xre/bin/xpcshell', '-g',
> '/builds/slave/test/build/xre/bin', '-v', '170', '-f', './httpd.js', ...
> 
> but if I use those paths for xpcshell and httpd.js during b2g reftests,
> httpd.js is not found:

I don't think httpd.js is in the bundle on runtime-binaries, so I'm not surprised that it's not working (though not sure why it would work for mochitests?). B2G reftests just use the remote reftest server, so currently it would be getting httpd.js from tests.zip. We can either add httpd.js to the runtime-binaries zip or we can make b2g use hostutils instead.
Flags: needinfo?(ahalberstadt)
Depends on: 885512
In bug 885512, httpd.js was added to the runtime-binaries xre zip, allowing the b2g httpd.js path change to work:

https://tbpl.mozilla.org/?tree=Try&rev=08eeacb8f09d
Assignee: nobody → gbrown
\o/
https://hg.mozilla.org/mozilla-central/rev/794e53a1c8dc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Reviewing logs this morning, I see that not all of the paths are as intended: No harm done, but the job is not complete.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The mochitest-remote problem was that options.httpdPath = '.' was setting the path too early. This patch removes that, moves the httpd-path option from MochitestRemote to Mochitest (since the server is in Mochitest and needs to check the option), and then uses '.' as a default when the option has not been set otherwise.

I also added abspath calls to make the httpd path more explicit/obvious in logs.

Waiting for new try results here: https://tbpl.mozilla.org/?tree=Try&rev=13c67b525d48
Attachment #768597 - Flags: review?(jmaher)
Comment on attachment 768597 [details] [diff] [review]
fix httpdPath for remote mochitest

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

Sorry -- problems on try.
Attachment #768597 - Flags: review?(jmaher)
The mochitest-remote problem in the landed patch was that options.httpdPath = '.' was setting the path too early. This patch removes that, moves the httpd-path option from MochitestRemote to Mochitest (since the server is in Mochitest and needs to check the option), and then uses '.' as a default only when the option has not been set otherwise.

The problem in the previous version of this patch was that options.utilityPath was being referenced too early. It turns out that the initial setting of options.utilityPath is incorrect!

I also added abspath calls to make the httpd path more explicit/obvious in logs.

Grand try run shows correct httpd.js paths: https://tbpl.mozilla.org/?tree=Try&rev=5f4b353b59f1

Finally!
Attachment #768597 - Attachment is obsolete: true
Attachment #768904 - Flags: review?(jmaher)
Comment on attachment 768904 [details] [diff] [review]
(2) fix httpdPath for remote mochitest

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

ahh, moving it to runtests vs runtestsremote solves the problem for reftest.  Glad to see this getting fixed!
Attachment #768904 - Flags: review?(jmaher) → review+
https://hg.mozilla.org/mozilla-central/rev/d04d512c25d9
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.