Closed Bug 1028407 Opened 10 years ago Closed 10 years ago

Try to make marionette connect to browser sessions quicker

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: gps, Assigned: chmanchester)

References

Details

(Keywords: pi-marionette-runner, pi-marionette-server, Whiteboard: [affects=loop])

Attachments

(1 file, 6 obsolete files)

As I'm working on bug 940954, I'm noticing that it seems to take longer-than-expected for Firefox to start and for Marionette to establish a session to it. Since Firefox appears initialized for a few seconds before Marionette does its thing, I suspect it is something in Marionette land.

It's slow enough that it will add considerable latency to tests that perform browser restarts and will make testing very annoying in terms of interrupting the developer change-test cycle.

I'm filing a bug to have someone look into the slowness so it can be improved, if possible. Maybe jgriffin, mdas, others have hunches on what the culprits are?
looks like it's this line:

https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py?rev=f5d1163268ae#571

We wait 5 seconds after we receive a message from the marionette server. This was likely a hacky way to wait until gecko was completely ready (the marionette-server starts running after profile-after-change, ie: at startup). 

We should instead set up the server (or respond only) when gecko is completely ready. We might be able to fix this by starting the server at final-ui-startup, but apparently there were issues with that (https://mxr.mozilla.org/mozilla-central/source/testing/marionette/components/marionettecomponent.js?rev=8a9d5e3bcc6b#85) which we should investigate. Or we can find a way to discern when gecko is ready from within the server, and only respond when we know we're ready to go.
Whiteboard: [affects=loop]
There was a 5 second sleep() in the Marionette client that made
establishing connections to servers take a long time. The history of
this value is unclear, but bug comments seem to indicate that Gecko
wasn't completely initialized during the observer notification that
starts the server.

We insert some code to let the JavaScript event loop spin for a few
ticks before starting the Marionette server. This should hopefully give
Gecko enough time to initialize.
Attachment #8450669 - Flags: review?(jgriffin)
Assignee: nobody → gps
Status: NEW → ASSIGNED
mdas: comment #1 did not make sense to me. You said "We might be able to fix this by starting the server at final-ui-startup" but my reading of the code is that we already do wait until final-ui-startup to start the server (this.init()).

Anyway, I changed my patch to remove the sleep and the world didn't blow up. I suppose I'll have to push this to Try and retrigger a few times to know for sure?
And my bzpost Mercurial extension automatically notified on Try push. What a pleasant surprise \o/

http://gregoryszorc.com/blog/2014/06/30/update-bugzilla-automatically-on-push/
Comment on attachment 8450669 [details] [diff] [review]
Establish connection to server faster

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

I traced the introduction of sleep(5) back to May 2012.  It's not clear why it was added, but I suspect it's because we weren't waiting for browser initialization correctly back then, but we are now.

In any case, try looks good even with retriggers, so I'm thinking this is safe.
Attachment #8450669 - Flags: review?(jgriffin) → review+
(In reply to Gregory Szorc [:gps] from comment #3)
> mdas: comment #1 did not make sense to me. You said "We might be able to fix
> this by starting the server at final-ui-startup" but my reading of the code
> is that we already do wait until final-ui-startup to start the server
> (this.init()).
> 
Ah, right, I misread it, thanks.
This seems to have caused Android 4.0 and B2G emulator mochitests and reftests bustage. I'm not entirely sure if they're related or an infra failure. I'm going to try a back out unless I hear otherwise. Failure, in case you're curious

0:04:36     INFO - Running tests...
00:04:36     INFO - Running tests...
00:04:36     INFO - logcat.py output:
00:04:36     INFO - reconnecting socket
00:04:36     INFO - 
00:04:36     INFO - 07/04/2014 23:55:39: DEBUG: logcat.py running SUT command: execext su t=3600 logcat -v time
00:04:36     INFO - 
00:04:36     INFO - Running post-action listener: _resource_record_post_action
00:04:36     INFO - #####
00:04:36     INFO - ##### Running close-request step.
00:04:36     INFO - #####
00:04:36     INFO - Running pre-action listener: _resource_record_pre_action
00:04:36     INFO - Running main action method: close_request
00:04:36 CRITICAL - Bad return status from http://mobile-imaging-008.p8.releng.scl1.mozilla.com/api/request/1659890/return/: 500!
00:04:36     INFO - Running post-action listener: _resource_record_post_action
00:04:36    FATAL - Uncaught exception: Traceback (most recent call last):
00:04:36    FATAL -   File "/builds/panda-0704/test/scripts/mozharness/base/script.py", line 1230, in run
00:04:36    FATAL -     self.run_action(action)
00:04:36    FATAL -   File "/builds/panda-0704/test/scripts/mozharness/base/script.py", line 1172, in run_action
00:04:36    FATAL -     self._possibly_run_method(method_name, error_if_missing=True)
00:04:36    FATAL -   File "/builds/panda-0704/test/scripts/mozharness/base/script.py", line 1113, in _possibly_run_method
00:04:36    FATAL -     return getattr(self, method_name)()
00:04:36    FATAL -   File "scripts/scripts/android_panda.py", line 473, in close_request
00:04:36    FATAL -     mph.close_request(self.request_url)
00:04:36    FATAL -   File "/builds/panda-0704/test/build/venv/lib/python2.7/site-packages/mozpoolclient.py", line 258, in close_request
00:04:36    FATAL -     check_mozpool_status(status)
00:04:36    FATAL -   File "/builds/panda-0704/test/build/venv/lib/python2.7/site-packages/mozpoolclient.py", line 57, in check_mozpool_status
00:04:36    FATAL -     raise MozpoolException('mozpool status not ok, code %s' % pprint.pformat(status))
00:04:36    FATAL - MozpoolException: mozpool status not ok, code 500
00:04:36    FATAL - Running post_fatal callback...
00:04:36    FATAL - Exiting -1
00:04:36     INFO - Running post-run listener: _resource_record_post_run
00:04:36     INFO - Running post-run listener: _upload_blobber_files
00:04:36     INFO - Blob upload gear active.
00:04:36     INFO - Preparing to upload files from /builds/panda-0704/test/build/blobber_upload_dir.
00:04:36     INFO - Files from /builds/panda-0704/test/build/blobber_upload_dir are to be uploaded with <fx-team> branch at the following location(s): https://blobupload.elasticbeanstalk.com
00:04:36     INFO - Running command: ['/builds/panda-0704/test/build/venv/bin/python', '/builds/panda-0704/test/build/venv/bin/blobberc.py', '-u', 'https://blobupload.elasticbeanstalk.com', '-a', '/builds/panda-0704/test/oauth.txt', '-b', 'fx-team', '-d', '/builds/panda-0704/test/build/blobber_upload_dir', '--output-manifest-url', '/builds/panda-0704/test/build/blobber_manifest.txt']
00:04:36     INFO - Copy/paste: /builds/panda-0704/test/build/venv/bin/python /builds/panda-0704/test/build/venv/bin/blobberc.py -u https://blobupload.elasticbeanstalk.com -a /builds/panda-0704/test/oauth.txt -b fx-team -d /builds/panda-0704/test/build/blobber_upload_dir --output-manifest-url /builds/panda-0704/test/build/blobber_manifest.txt
00:04:37     INFO -  (blobuploader) - INFO - Open directory for files ...
00:04:37     INFO -  (blobuploader) - INFO - Uploading /builds/panda-0704/test/build/blobber_upload_dir/logcat.log ...
00:04:37     INFO -  (blobuploader) - INFO - Using https://blobupload.elasticbeanstalk.com
00:04:37     INFO -  (blobuploader) - INFO - Uploading, attempt #1.
00:04:38     INFO -  (blobuploader) - INFO - TinderboxPrint: <a href='http://mozilla-releng-blobs.s3.amazonaws.com/blobs/fx-team/sha512/9a890619fa603f89de8bc645e89a53ed182816831376a9652cbfcd1ac280e0f3055368604a6b757c62c3b8a7081f3ef2d5af8a419f901ee5d8333493c30744ec'>logcat.log</a>: uploaded
00:04:38     INFO -  (blobuploader) - INFO - Blobserver returned 202. File uploaded!
00:04:38     INFO -  (blobuploader) - INFO - Done attempting.
00:04:38     INFO -  (blobuploader) - INFO - Uploading to blobber a json structured file with all the files that have been uploaded.
00:04:38     INFO -  (blobuploader) - INFO - Here are the contents: {"logcat.log": "http://mozilla-releng-blobs.s3.amazonaws.com/blobs/fx-team/sha512/9a890619fa603f89de8bc645e89a53ed182816831376a9652cbfcd1ac280e0f3055368604a6b757c62c3b8a7081f3ef2d5af8a419f901ee5d8333493c30744ec"}
00:04:38     INFO -  (blobuploader) - INFO - Uploading uploaded_files.json ...
00:04:38     INFO -  (blobuploader) - INFO - Using https://blobupload.elasticbeanstalk.com
00:04:38     INFO -  (blobuploader) - INFO - Uploading, attempt #1.
00:04:39     INFO -  (blobuploader) - INFO - TinderboxPrint: <a href='http://mozilla-releng-blobs.s3.amazonaws.com/blobs/fx-team/sha512/d1a09d85e4827a9acd9ce7effd29d32717aabca59a216496396210d8e8c3a9966e21e62eded45d0a72b0d43deeaac976b28aac5c65f034f3033720ef6f7e36db'>uploaded_files.json</a>: uploaded
00:04:39     INFO -  (blobuploader) - INFO - Blobserver returned 202. File uploaded!
00:04:39     INFO -  (blobuploader) - INFO - Done attempting.
00:04:39     INFO -  (blobuploader) - INFO - Iteration through files over.
00:04:39     INFO - Return code: 0
On second thoughts, this seems to be infra. I can reproduce on retriggered greens on an earlier push. Sorry for the noise :)
Backed out in https://hg.mozilla.org/integration/fx-team/rev/26cefac4dd83 for emulator mochitest failures.
Boo. I have no clue how to start debugging this since I don't know anything about the FxOS testing infrastructure. I don't suppose I could get some help?

Fortunately, this patch isn't super critical. It would be nice though. I'm tempted to add the sleep(5) back in for the emulator, as hacky as that sounds.
Oops, just to clarify. The test failure I pasted in is not the reason for the backout.
This problem might not be because of the removal of the 5 second sleep, but because we no longer look for ":" in the output.

For B2G, we set up adb port forwarding, and once setup, that will allow a socket connection to succeed even when Marionette isn't running.  It might be worth adding back the check for ":" and see if that resolves the problem.
I'm not working on this, sadly.
Assignee: gps → nobody
Status: ASSIGNED → NEW
This has been bothering me so I pushed to try with jgriffin's suggestion from comment 14.

Mn with some retriggers:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=86355d303ac3

Emulator mochitests:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7bac9d4991ec
The orange on the emulator mochitests looks like bug 879717.
Based on the try runs linked in comment 16 this looks good to go, but maybe there are more platforms/suites we should check? Removing the code from the previous version of the patch to re-dispatch init() in marionettecomponent.js didn't seem to hurt anything, either.
Attachment #8505907 - Flags: review?(jgriffin)
Assignee: nobody → cmanchester
Status: NEW → ASSIGNED
(In reply to Chris Manchester [:chmanchester] from comment #16)
> This has been bothering me so I pushed to try with jgriffin's suggestion
> from comment 14.
> 
> Mn with some retriggers:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=86355d303ac3
> 
> Emulator mochitests:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7bac9d4991ec

I've retriggered opt mochitest chunk 3 a number of times so we can convince ourselves that the orange there (and in debug chunk 7) isn't a perma-fail.  I doubt it is.
(In reply to Jonathan Griffin (:jgriffin) from comment #19)
> (In reply to Chris Manchester [:chmanchester] from comment #16)
> > This has been bothering me so I pushed to try with jgriffin's suggestion
> > from comment 14.
> > 
> > Mn with some retriggers:
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=86355d303ac3
> > 
> > Emulator mochitests:
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7bac9d4991ec
> 
> I've retriggered opt mochitest chunk 3 a number of times so we can convince
> ourselves that the orange there (and in debug chunk 7) isn't a perma-fail. 
> I doubt it is.

I think I picked up a failure that's been backed out. Here's a fresh try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a0a8cb006472
Comment on attachment 8505907 [details] [diff] [review]
Establish connection to server faster.

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

Looks great, and try is happy now, thanks!
Attachment #8505907 - Flags: review?(jgriffin) → review+
(In reply to Wes Kocher (:KWierso) from comment #23)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5aea4fbbb47d for
> being the likely cause of web-platform-test bustage.

Looks like this was the culprit. Lots of this in the busted logs:

12:21:21     INFO - PROCESS | 15095 | JavaScript error: http://web-platform.test:8000/resources/testharnessreport.js, line 7: Error: Permission denied to access property 'timeout_multiplier'
12:21:31     INFO - PROCESS | 15095 | MARIONETTE LOG: INFO: Timeout fired
12:21:31     INFO - PROCESS | 15095 | JavaScript error: dummy file, line 21: Error: Permission denied to access property 'timeout'


...which explains the timeouts pretty well. Something about how web-platform-tests use marionette (a lot can happen in 5 seconds). James, any ideas?
Flags: needinfo?(james)
Ok, I tracked this down. I'll post details and a patch tomorrow.
Flags: needinfo?(james)
With the removal of a 5 second sleep in marionette's python client, the wpt harness
interleaved the "about:blank" load event with the load of the harness start page, causing
a security exception due to an incorrect principal set in marionette's execution sanbox
for content scripts. Moving the load of "about:blank" from the commandline of the
firefox process to the "startup.homepage_welcome_url" pref avoided this interleaving.

On try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=99888b62c7fb
Attachment #8507902 - Flags: review?(james)
Comment on attachment 8507902 [details] [diff] [review]
Set the welcome homepage url for wpt to about:blank via a pref rather than a commandline option.

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

r+ for the change if it works, which I haven't verified. It feels like it's fundamentally a bug in marionette if you can't specify a url on the command line though.

r- for the location; you need to patch https://github.com/w3c/wptrunner and then we need to update from upstream (but a try run with this applied to m-c would be appreciated)
Attachment #8507902 - Flags: review?(james) → review-
(In reply to James Graham [:jgraham] from comment #27)
> Comment on attachment 8507902 [details] [diff] [review]
> Set the welcome homepage url for wpt to about:blank via a pref rather than a
> commandline option.
> 
> Review of attachment 8507902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ for the change if it works, which I haven't verified. It feels like it's
> fundamentally a bug in marionette if you can't specify a url on the command
> line though.

This was tested locally and on try, see comment 26. The responsibility for ensuring the browser is ready doesn't seem completely obvious in the case the binary isn't passed into marionette (the load of a page specified on the commandline happens a lot later than I would expect). Until now that responsibility has been taken on by marionette implicitly with a 5 second sleep, which is truly worth avoiding.

> 
> r- for the location; you need to patch https://github.com/w3c/wptrunner and
> then we need to update from upstream (but a try run with this applied to m-c
> would be appreciated)

Will do. See comment 26.
Right, sorry I missed the try run. The other comments still apply; I think we should file a separate bug on having marionette wait for any load from the command line (without using sleep), as requiring you to not have one seems like an obvious sharp edge.
(In reply to James Graham [:jgraham] from comment #29)
> Right, sorry I missed the try run. The other comments still apply; I think
> we should file a separate bug on having marionette wait for any load from
> the command line (without using sleep), as requiring you to not have one
> seems like an obvious sharp edge.

If this breaks others I would think it's worth blocking on.
Based on some feedback on the marionette mailing list [1], this isn't a palatable change to start up behaviour. We need to look into this in greater depth before landing.

[1] https://groups.google.com/forum/#!forum/mozilla.tools.marionette
Will the patches for this bug address the problem where we let the client interact with content before the content is ready? If not, a separate bug should be filed for that case. It's extremely useful to have that, and from what I can tell, it would resolve the conflict between getting a session and navigating mentioned in  https://groups.google.com/forum/#!forum/mozilla.tools.marionette
(In reply to Malini Das [:mdas] from comment #32)
> Will the patches for this bug address the problem where we let the client
> interact with content before the content is ready? If not, a separate bug
> should be filed for that case. It's extremely useful to have that, and from
> what I can tell, it would resolve the conflict between getting a session and
> navigating mentioned in 
> https://groups.google.com/forum/#!forum/mozilla.tools.marionette

I'm not sure, but this sounds related and could be impacted by bug 1046512 as well. Is this issue documented somewhere I could check out? Thanks!
Flags: needinfo?(mdas)
That's the origin of the 5 second wait we had in the client code. Originally, if you tried to make a new session too soon after launch, then the tests would hang because of a gecko error. You can test it out with b2g desktop or a device. For b2g desktop, try to start a marionette session as soon as you create the running instance. It will just hang on that request, and if you look at the gecko log, you'll get js errors. In my case, I got System JS : ERROR chrome://marionette/content/marionette-listener.js:4 - NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsISyncMessageSender.sendSyncMessage] but the error depends on your timing. On device, do 'adb shell stop b2g' and then 'adb shell start b2g' and immediately try to connect. You'll have the same problem. I haven't seen this on desktop recently, but I think it will show up on tbpl since they are slower computers. You *might* be able to reproduce this with firefox desktop given a slow enough computer...

The problem is that we let users connect to marionette too early. We start a server on the port at gecko startup, when not everything in the chrome context is loaded, and when the content processes may not be ready to be interacted with. We should instead only accept connections from clients when the content is ready. We should send back 'not ready' when a client tries to create a newsession if the content is not yet ready.
Flags: needinfo?(mdas)
(In reply to Malini Das [:mdas] from comment #34)
> That's the origin of the 5 second wait we had in the client code.
> Originally, if you tried to make a new session too soon after launch, then
> the tests would hang because of a gecko error. You can test it out with b2g
> desktop or a device. For b2g desktop, try to start a marionette session as
> soon as you create the running instance. It will just hang on that request,
> and if you look at the gecko log, you'll get js errors. In my case, I got
> System JS : ERROR chrome://marionette/content/marionette-listener.js:4 -
> NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff
> (NS_ERROR_UNEXPECTED) [nsISyncMessageSender.sendSyncMessage] but the error
> depends on your timing. On device, do 'adb shell stop b2g' and then 'adb
> shell start b2g' and immediately try to connect. You'll have the same
> problem. I haven't seen this on desktop recently, but I think it will show
> up on tbpl since they are slower computers. You *might* be able to reproduce
> this with firefox desktop given a slow enough computer...
> 
> The problem is that we let users connect to marionette too early. We start a
> server on the port at gecko startup, when not everything in the chrome
> context is loaded, and when the content processes may not be ready to be
> interacted with. We should instead only accept connections from clients when
> the content is ready. We should send back 'not ready' when a client tries to
> create a newsession if the content is not yet ready.

The patches in this bug don't solve an underlying issue. Both attempts to land have consisted of more or less just removing the sleep, justified by passing tests on all platforms in automation but failing due to harness-specific issues. So it sounds like either the underlying issue has been dealt with in gecko in the interim, or automation isn't failing due to it. In any event, at least the page loading issue would need to be resolved before re-landing.
There's a later event, "browser-delayed-startup-finished", that if we block for handles the w-p-t case fine. I don't think it makes sense to land anything like this until we have a better idea of how this impacts b2g or relates to the issues seen there.
Attachment #8505907 - Attachment is obsolete: true
Attachment #8507902 - Attachment is obsolete: true
For Mozmill tests we have lots of restarts. So the current delay adds a significant amount of extra time to run all of them. So it would be great to have this fixed for bug 1080766.
Blocks: m21s
I ran this through try the other day and things look ok:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c4a548447b90

ato, maybe you can take a look at this?
Attachment #8517485 - Flags: review?(ato)
Attachment #8510380 - Attachment is obsolete: true
Comment on attachment 8517485 [details] [diff] [review]
Establish connection to server faster

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

Overall looks good, but I have a few questions:

::: testing/marionette/client/marionette/marionette.py
@@ +597,5 @@
>                      return True
>              except socket.error:
>                  pass
> +            finally:
> +                if sock:

Generally I'd prefer if sock is not None, but in this specific context this is fine.

@@ +599,5 @@
>                  pass
> +            finally:
> +                if sock:
> +                    sock.close()
> +            time.sleep(.1)

What's the background for the 0.1 second sleep?

::: testing/marionette/marionette-server.js
@@ +80,5 @@
>  }, "system-message-listener-ready", false);
>  
> +// This is used on desktop to prevent newSession from returning before a page
> +// load initiated by the Firefox command line has completed.
> +let gHasDelayedBrowserStartup = false;

I first misinterpreted this to mean “does the browser have the feature browser-delayed-startup-finished”.  

Could we rename it to “delayedBrowserReady”, or something similar, to indicate that the event has fired.

This would be more in line with the other observers in this file.

@@ +584,5 @@
> +    function runSessionStart() {
> +      if (!Services.prefs.getBoolPref("marionette.contentListener")) {
> +        waitForWindow.call(this);
> +      }
> +      else if ((appName != "Firefox") && (this.curBrowser == null)) {

!== and ===, especially when comparing null values

@@ +600,3 @@
>      }
> +
> +    if (!gHasDelayedBrowserStartup && (appName != "B2G")) {

!==

@@ +605,5 @@
> +        if (!browserReady) {
> +          browserReady = true;
> +          runSessionStart.call(this);
> +        }
> +      }.bind(this), DELAYED_STARTUP_FINISH, false);

So we add a second observer here which calls runSessionStart when browser-delayed-startup-finished is set to true.  Why do we keep track of it globally with gHasDelayedBrowserStartup too?

Could we get away with blocking in this function until this event is fired?
Attachment #8450669 - Attachment is obsolete: true
(In reply to Andreas Tolfsen (:ato) from comment #39)
> Comment on attachment 8517485 [details] [diff] [review]
> Establish connection to server faster
> 
> Review of attachment 8517485 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good, but I have a few questions:
> 
> ::: testing/marionette/client/marionette/marionette.py
> @@ +597,5 @@
> >                      return True
> >              except socket.error:
> >                  pass
> > +            finally:
> > +                if sock:
> 
> Generally I'd prefer if sock is not None, but in this specific context this
> is fine.
> 
> @@ +599,5 @@
> >                  pass
> > +            finally:
> > +                if sock:
> > +                    sock.close()
> > +            time.sleep(.1)
> 
> What's the background for the 0.1 second sleep?
It's an arbitrary polling interval. I guess it's a balance between making things appear lively and letting something happen between iterations.
> 
> ::: testing/marionette/marionette-server.js
> @@ +80,5 @@
> >  }, "system-message-listener-ready", false);
> >  
> > +// This is used on desktop to prevent newSession from returning before a page
> > +// load initiated by the Firefox command line has completed.
> > +let gHasDelayedBrowserStartup = false;
> 
> I first misinterpreted this to mean “does the browser have the feature
> browser-delayed-startup-finished”.  
> 
> Could we rename it to “delayedBrowserReady”, or something similar, to
> indicate that the event has fired.
Will do!
> 
> This would be more in line with the other observers in this file.
> 
> @@ +584,5 @@
> > +    function runSessionStart() {
> > +      if (!Services.prefs.getBoolPref("marionette.contentListener")) {
> > +        waitForWindow.call(this);
> > +      }
> > +      else if ((appName != "Firefox") && (this.curBrowser == null)) {
> 
> !== and ===, especially when comparing null values
appName comes from an interface with types, so that will always be a string, but agreed on curBrowser. This was here before, so I would wonder about existing weird behavior, but should be ok.
> 
> @@ +600,3 @@
> >      }
> > +
> > +    if (!gHasDelayedBrowserStartup && (appName != "B2G")) {
> 
> !==
> 
> @@ +605,5 @@
> > +        if (!browserReady) {
> > +          browserReady = true;
> > +          runSessionStart.call(this);
> > +        }
> > +      }.bind(this), DELAYED_STARTUP_FINISH, false);
> 
> So we add a second observer here which calls runSessionStart when
> browser-delayed-startup-finished is set to true.  Why do we keep track of it
> globally with gHasDelayedBrowserStartup too?
> 
> Could we get away with blocking in this function until this event is fired?

Nothing ensures startSession is called before the notification is sent, so we could hang if we block here. Keeping a separate flag is a little ugly, I think removing the observer would work just as well.
There was a 5 second sleep() in the Marionette client. The history of
its value is unclear, but bug comments seem to indicate that Gecko
wasn't completely initialized during the observer notification that
starts the server. We removed the sleep.
Attachment #8518239 - Flags: review?(ato)
Attachment #8517485 - Attachment is obsolete: true
Attachment #8517485 - Flags: review?(ato)
Comment on attachment 8518239 [details] [diff] [review]
Establish connection to server faster

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

lgtm
Attachment #8518239 - Flags: review?(ato) → review+
Updated commit message for checkin.
Attachment #8518239 - Attachment is obsolete: true
Comment on attachment 8518327 [details] [diff] [review]
Establish connection to server faster.

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

r=ato
Attachment #8518327 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/113c36ec9ed1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.