Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

With bug 923607 being implemented the safe mode dialog should be suppressed, but as it looks like this is not happening. Instead the dialog stays around forever, and finally the application quits with:

Traceback (most recent call last):

  File "/Volumes/data/code/gecko/testing/marionette/harness/marionette_harness/runtests.py", line 92, in cli
    failed = harness_instance.run()

  File "/Volumes/data/code/gecko/testing/marionette/harness/marionette_harness/runtests.py", line 72, in run
    runner.run_tests(tests)

  File "/Volumes/data/code/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 842, in run_tests
    self.marionette = self.driverclass(**self._build_kwargs())

  File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/marionette.py", line 615, in __init__
    self.raise_for_port(timeout=self.startup_timeout)

  File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/decorators.py", line 28, in _
    m._handle_socket_failure()

  File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/decorators.py", line 23, in _
    return func(*args, **kwargs)

  File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/marionette.py", line 701, in raise_for_port
    self.host, self.port))

IOError: Process killed because the connection to Marionette server is lost. Check gecko.log for errors (Reason: Timed out waiting for connection on localhost:2828!)

What I think is that delaying the initialization code of Marionette from "profile-after-change" to "sessionstore-windows-restored" caused this problem.

I got informed about this failure by Blake while he was helping to write a test for bug 1219725, and after a restart of Firefox the safe mode surprisingly gets started.

Geoff, maybe this could be one of the reason why we fail that much in Marionette? Not sure why the safe mode actually gets started.

	Blake Winton (:bwinton) (:☕️)
	Comment 1 • 7 years ago

It might be useful to note that in the test we're running, we don't restore the session until much later, since it's the session restore functionality that we're trying to test, so waiting for "sessionstore-windows-restored" is probably not going to work out well.  ;)

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 7 years ago

The command to reproduce this behavior is the following:

> mach marionette-test --app-arg="-safe-mode"

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 7 years ago

So the start of Marionette has been delayed by bug 1315611 and even further with bug 1369700. As discussed earlier "sessionstore-windows-restored" should be still alright, even for tests which test sessionrestore. The observer notification is also fired before you click on restore previous session or whatever ui element.

I wonder if we should return to use "profile-after-change" to initialize Marionette and to be able to close the safe mode dialog during startup. But tests should not start before everything is ready.

Gijs and Florian, would that be ok?

	Florian Quèze [:florian]
	Comment 4 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #3)

> I wonder if we should return to use "profile-after-change" to initialize
> Marionette and to be able to close the safe mode dialog during startup. But
> tests should not start before everything is ready.
> 
> Gijs and Florian, would that be ok?

Can you be more specific about what you mean by "use "profile-after-change" to initialize Marionette"? If you are talking about running a dozen lines of code, that's probably fine. If it's importing several JS modules, it's not.

	:Gijs (he/him)
	Comment 5 • 7 years ago

If we only need to do this in the safe mode case, I expect what we should do is also initialize when the safe mode dialog comes up, not always initialize earlier.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 7 years ago

Thank you both for the replies. I had another look into the Marionette component code, and as it looks like we can scratch that theory with the delayed initialization of Marionette. This is not the cause of this regression! But changes for `this.enabled` actually caused this, because we never run the underlying code to handle the safe mode dialog anymore:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/components/marionette.js#174-180

At this stage `this.enabled` is false.

I will check what's going wrong and a patch should be simple enough. As of now both builds on central and beta are affected.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 7 years ago

Marionette registers itself as a component with a command line handler. But surprisingly the `handle` method never gets executed, and as such `this.enabled` is false, unless the MOZ_MARIONETTE environment variable is set.

The regression range is here:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d7e40bb852ea047a0e5f530bcdc04d29a1765001&tochange=cd8a44efa4227dff465187c07bcc6789f9d2d603

And nothing else beside bug 1355888 (Prevent Marionette TCP server from starting/stopping on flipping marionette.enabled) strikes out here.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 8 • 7 years ago

Ok, so it's not a regression per se. The problem is that only in safe mode the registered command line handler is not obeyed. When I run Firefox normally I can see that our `handle` function is getting executed, but with the `-safe-mode` argument, the method isn't called.

The patch as pointed out above, changed the way how we enable Marionette. So even with that in place the `handle` method is not called. Are we doing something special for command line args when starting Firefox in safe mode?

Benjamin, do you know?

	Benjamin Smedberg
	Comment 9 • 7 years ago

I don't know but I can point you where to debug this. We run command lines through nsCommandLine::Run and in this case you are a handler so you should be showing up in the nsCommandLine::EnuemrateHandlers method.

https://dxr.mozilla.org/mozilla-central/rev/e1e4a481b7e88dce163b9cccc2fb72032023befa/toolkit/components/commandlines/nsCommandLine.cpp#502

So I'd step through that to see:
1) whether the marionette handler is showing up at all from the category manager
2) whether this code is able to get the service
3) whether the ->Handle method at https://dxr.mozilla.org/mozilla-central/rev/e1e4a481b7e88dce163b9cccc2fb72032023befa/toolkit/components/commandlines/nsCommandLine.cpp#599 is succeeding or failing.

	Mike Taylor [:miketaylr]
	Comment 10 • 7 years ago

Let's not track as a regression, based on comment 8. Henrik, did you get a chance to look at Comment #9?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 11 • 7 years ago

Thanks Benjamin for the helpful information. After I added a couple of further logging statements to that file, I was able to see that we indeed call `handle` for each registered command line handler. BUT, this method gets called after the safe mode dialog has been closed, and not before. It means that Marionette won't be initialized.

Then I found the following lines of code:
https://dxr.mozilla.org/mozilla-central/rev/9af23c413a1f8d337b19b4f8450e241e91b71136/toolkit/xre/nsAppRunner.cpp#4447-4453

What's up with this "command-line-startup" observer notification? It's getting sent here, but not a single file in mozilla-central makes use of it. I tried to use it for Marionette, and it actually works pretty fine. Means Marionette is getting enabled before the safe mode dialog pops-up, and can close it.

Now I wonder if Marionette could be such a special-cased service, or if we simply run the command line handler code too late during startup. Maybe it should be run before the safe-mode dialog is shown?

I will attach a patch which would make it work with the above logic.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 15 • 7 years ago

I talked with Benjamin and as of now there is no simple solution around. To get this finally working without the "command-line-startup" notification we would have to implement the command line handler via C++. Benjamin is happy with taking the proposed method for now.

	Andreas Tolfsen ❲:ato❳
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8882499 [details]
Bug 1374762 - Allow Marionette to handle the safe mode dialog.

https://reviewboard.mozilla.org/r/153616/#review158932

::: testing/marionette/components/marionette.js:251
(Diff revision 3)
>  
>  MarionetteComponent.prototype.suppressSafeModeDialog = function (win) {
>    win.addEventListener("load", () => {
>      if (win.document.getElementById("safeModeDialog")) {
>        // accept the dialog to start in safe-mode
> +      this.logger.debug("Safe Mode detected. Going to suspress the dialog now.")

Missing semicolon.

	Pulsebot
	Comment 18 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05a807bd0010
Allow Marionette to handle the safe mode dialog. r=ato

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 19 • 7 years ago

Backed out for eslint issues https://hg.mozilla.org/integration/autoland/rev/25cac3ac73759dfffd18b4b613053959af3fd6b4

https://treeherder.mozilla.org/logviewer.html#?job_id=111234798&repo=autoland

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 21 • 7 years ago

I missed a rebase and as such the recent changes weren't part of my tree and got not obeyed by the linter.

	Pulsebot
	Comment 22 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5730b90bc7ff
Allow Marionette to handle the safe mode dialog. r=ato

	Phil Ringnalda (:philor)
	Comment 23 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5730b90bc7ff

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 24 • 7 years ago

Please uplift this test-only patch to beta which seems to help reducing intermittent failures. Thanks.

	Carsten Book [:Tomcat]
	Comment 25 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/1a6afc516f27