1009224 - sleep() and waitFor() keeps Firefox running, even it should quit (causing long shutdown times)

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: test2.js

As what I have seen is that with a Mozmill test, which is forcing the application to quit, we can delay the shutdown of Firefox to whatever time we want. Reason is that even inside the test we force a quit, the script gets continued until the next action is finished. This may take >30s and more, depending on which command is executed.

In this case it is waitForPageLoad(5000), triggered after hitting Accel+Q. Telemetry reports a shutdown time of 5s.

Steps to reproduce:
1. Download the mozmill-environment: https://mozqa.com/mozmill-env/
2. Extract and source into via 'source ../mozmill-env/run
3. Save the attachment
4 [review]. Run Mozmill: 'mozmill -b path_to_binary -t test2.js

I'm not sure why the mozmill script is not getting stopped immediately. Tim, could you have a look at with a debugger? I'm not sure how to get the appropriate information.

Is it our fault, or should Firefox kill the currently running script directly?

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #0)
> 4 [review]. Run Mozmill: 'mozmill -b path_to_binary -t test2.js

You could even add '--debugger gdb' as I remembered now. So the output of gdb for the remaining threads is:

(gdb) thread list
[Switching to thread 1 (Thread 0x7ffff7fcc780 (LWP 20391))]
#1  0x00007ffff67ea524 in PR_WaitCondVar () from /mozilla/bin/release/libnspr4.so

Maybe we are waiting for the socket as being created by jsbridge to be closed? It looks like, given that this is the last remaining thread for the process.

When I wait a bit longer, and don't press ctrl+c, I even get:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff1cc0c7f in js::jit::CompactBufferReader::readFixedUint32_t() () from /mozilla/bin/nightly/libxul.so

Maybe that is a different issue.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so the same slowness during shutdown also happens with Mozmill 1.5, which is not based on a NSPR socket communication. The above seen remaining thread for NSPR4 might be a side-effect with Mozmill 2.0. The crash I might have to file as a separate bug.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

I don't know enough about our shutdown process, unfortunately. I wonder who would be a good person to investigate this...

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As talked on IRC the problem here is Mozmill's waitFor() method:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/assertions.js#L573

It doesn't let Firefox shutdown until it really timed out or the expected state has been reached. Maybe all this is caused by spinning the event loop. So before we actually can start with bug 791960, a workaround has been proposed by Tim. Means we simply could check nsIAppStartup.shuttingDown inside the loop and break if it is true.

This temporary fix will be necessary for the methods sleep() and waitFor().

Once we have converted our code to task.jsm or something similar and we still face those issues, we might have to investigate it more. But for now this should be enough for our case.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

I just checked and this is exactly what the sync test code does, although it uses a "quit-application" observer but that doesn't matter. Should be the right thing to do as long as we need to keep spinning the event loop.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

With this workaround patch we can safely call sleep() and waitFor() at any time, and we will not cause a slow shutdown of Firefox.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1.1

Missed to reset the manifest file for Python tests.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8421606 [details] [diff] [review]
Patch v1.1

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

Seems like Andrew can't get to it right now. Given the importance of the patch, Dave volunteered to step in as reviewer. Thanks.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on master as:
https://github.com/mozilla/mozmill/commit/b5b7343aecbddec1460fcea4554bfbce79cb8a4c

After the push I have seen that I missed to update the commit message regarding the reviewer. Sorry, for still mentioning you Andrew and not Dave.