Closed Bug 1374762 Opened 2 years ago Closed 2 years ago

Starting Firefox in safe mode the registered command line handler of Marionette is not handled

Categories

(Testing :: Marionette, defect, major)

55 Branch
defect
Not set
major

Tracking

(firefox54 unaffected, firefox55 fixed, firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox54 --- unaffected
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

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.
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.  ;)
The command to reproduce this behavior is the following:

> mach marionette-test --app-arg="-safe-mode"
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?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(florian)
(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.
Flags: needinfo?(florian)
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.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Assignee: nobody → hskupin
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.
Blocks: 1355888
OS: Unspecified → All
Hardware: Unspecified → All
Version: 52 Branch → 55 Branch
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?
Flags: needinfo?(benjamin)
Summary: Starting Firefox in safe mode hangs Marionette → Starting Firefox in safe mode the registered command line argument of Marionette is not handled
Summary: Starting Firefox in safe mode the registered command line argument of Marionette is not handled → Starting Firefox in safe mode the registered command line handler of Marionette is not handled
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.
Flags: needinfo?(benjamin)
Let's not track as a regression, based on comment 8. Henrik, did you get a chance to look at Comment #9?
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.
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin) → needinfo?(benjamin)
Attachment #8882499 - Flags: review?(ato)
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.
Flags: needinfo?(benjamin)
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.
Attachment #8882499 - Flags: review?(ato) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05a807bd0010
Allow Marionette to handle the safe mode dialog. r=ato
I missed a rebase and as such the recent changes weren't part of my tree and got not obeyed by the linter.
Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5730b90bc7ff
Allow Marionette to handle the safe mode dialog. r=ato
https://hg.mozilla.org/mozilla-central/rev/5730b90bc7ff
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Please uplift this test-only patch to beta which seems to help reducing intermittent failures. Thanks.
Whiteboard: [checkin-needed-beta]
Whiteboard: [checkin-needed-beta]
Depends on: 1254136
You need to log in before you can comment on or make changes to this bug.