Improve Firefox browser process tracking #25644
Conversation
…er cmdline arg, and waiting longer for process startup.
|
So what is |
It keeps the spawned process alive until the browser closes. import subprocess, sys, time
proc = subprocess.Popen(['C:\\Program Files\\Mozilla Firefox\\firefox.exe', '-new-instance', '-wait-for-browser', 'https://wiki.mozilla.org/Firefox/CommandLineOptions'])
while True:
if proc.poll() is not None:
print('Browser process quit.')
sys.exit(0)
time.sleep(1)With Without |
|
I see. But it sounds like Is that a bug in Given that we still cannot just wait on the single PID, is there any advantage to using |
|
I think i'm not understanding the "since the process handle returned by subprocess.Popen() cannot be used to cascade the spawned Firefox windows." part |
It does. Seemingly so at least, in the small python test case that I posted above. Although I see CircleCI is giving some error, so I am running this on my CI also to test.
I am hoping that would not be the case. That is what I referred with the comment "It may be possible to remove the use of
The Emscripten test harness needs the process handles for two purposes:
The comment "since the process handle returned by subprocess.Popen() cannot be used to cascade the spawned Firefox windows." means that even with |
|
Ok great. lgtm either way. I guess we only need to keep the handle to the root processes then maybe we can do the cascading thing logically here in this function and avoid storing the extended process list in cls.browser_procs? i.e. for the purposes of kill in the browser we only need a single process now? |
| class FirefoxConfig: | ||
| data_dir_flag = '-profile ' | ||
| default_flags = ('-new-instance',) | ||
| default_flags = ('-new-instance', '-wait-for-browser') if WINDOWS else ('-new-instance',) |
There was a problem hiding this comment.
Probably warrants a comment here.
There was a problem hiding this comment.
Yeah, I'll add one. I am still testing to understand why this is necessary.
juj
force-pushed
the
firefox_harness_fixes
branch
from
891901c to
65fa8c8
Compare
In previous PR #25644 I added `-wait-for-browser` use on Windows to help single-threaded Windows runner not leave behind stale browser windows. For some reason, it was not passing on CircleCI on Linux. Testing this again to see what the failure is. In local testing with a test script: ```py import subprocess, sys, time cmd = ['C:\\Program Files\\Mozilla Firefox\\firefox.exe', '-new-instance', '-profile', sys.argv[1], '-wait-for-browser', 'https://wiki.mozilla.org/Firefox/CommandLineOptions'] print('Launching browser: ' + ' '.join(cmd)) proc = subprocess.Popen(cmd) for i in range(10): if proc.poll() is not None: print('Oops: process already quit, so we did not get a process handle to keep track of the browser instance.') sys.exit(0) time.sleep(1) if proc.poll() is None: print('terminating browser process') proc.terminate() time.sleep(2) if proc.poll() is None: print('terminating did not work. killing browser process') proc.kill() print('All done') ``` I am unable to find a behavioral difference between Windows and Linux. So testing this out again on CircleCI.
2 participants
Improve Firefox browser process tracking by using the -wait-for-browser cmdline arg, and waiting longer for process startup.
Unfortunately
-wait-for-browseris not enough to get rid of the process delta polling, since the process handle returned bysubprocess.Popen()cannot be used to cascade the spawned Firefox windows.Waiting for 2 seconds for Firefox startup was not enough, so make the wait time more conservative.
Add the detected processes to the list rather than replace, so that all process handles are retained.
It may be possible to remove the use of
launch_browser_harness_with_proc_snapshot_workaround()for headless Firefox runs, but more testing will be needed to verify that.