New issue
Advanced search Search tips

Issue 793668 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Task



Sign in to add a comment

Pass env vars to session_manager's EnableChromeTesting method and go back to generated bindings

Project Member Reported by derat@chromium.org, Dec 10 2017

Issue description

To make Chrome crash dumping more predictable for  issue 770562 , I'm updating session_manager's EnableChromeTesting D-Bus method to take a list of environment variables to set when restarting Chrome. (Specifically, I want to set HEADLESS and BREAKPAD_DUMP_LOCATION.)

login_manager is using generated D-Bus bindings, which don't support optional args, so I need to change session_manager to use:

  <annotation name="org.chromium.DBus.Method.Kind" value="raw" />

and deal with dbus::Message objects directly until all callers have been updated to pass the additional argument.

I'm filing this bug to track updating callers and switching back to the generated code. I think that testers run ToT Autotest-based tests against old system images, so I probably need to wait a few releases (M66-ish?) before I can update Telemetry (or whatever) to pass the additional arg.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/db015ab7cdeb9c579c370307fb9cadb27420fda1

commit db015ab7cdeb9c579c370307fb9cadb27420fda1
Author: Daniel Erat <derat@chromium.org>
Date: Mon Dec 11 20:47:50 2017

login: Make EnableChromeTesting take environment vars.

Update session_manager's EnableChromeTesting D-Bus method to
take an additional (currently optional) argument listing
environment variables that should be set before running
Chrome.

Unfortunately, D-Bus codegen doesn't support optional args,
so this requires manually unmarshaling arguments from calls
to this method until all callers have been updated.

BUG= chromium:770562 , chromium:793668 
TEST=tests pass; also verified that method works both with
     and without env vars arg
CQ-DEPEND=Ib65ead08400dc4af8c17040d3aa60e57b8295638

Change-Id: Ia1b7f030c2241c051233ccce38e1150a625f943a
Reviewed-on: https://chromium-review.googlesource.com/818737
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>

[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/session_manager_main.cc
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/mock_process_manager_service.h
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/session_manager_service.h
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/session_manager_service.cc
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/fake_browser_job.h
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/session_manager_impl.cc
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/dbus_bindings/org.chromium.SessionManagerInterface.xml
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/session_manager_impl_unittest.cc
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/browser_job.h
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/process_manager_service_interface.h
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/browser_job.cc
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/browser_job_unittest.cc
[modify] https://crrev.com/db015ab7cdeb9c579c370307fb9cadb27420fda1/login_manager/session_manager_impl.h

Comment 2 by derat@chromium.org, Dec 16 2017

Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8ae9ce6f6e92fd3b140219ac5684cb0c917dbcc8

commit 8ae9ce6f6e92fd3b140219ac5684cb0c917dbcc8
Author: Daniel Erat <derat@chromium.org>
Date: Wed Dec 20 20:58:30 2017

debugd: Pass env vars to session_manager.

Make debugd pass an (empty) extra_environment_variables
argument
when calling session_manager's EnableChromeTesting D-Bus
method. This argument is optional but will be required in
the future.

BUG= chromium:793668 
TEST=ran /usr/libexec/debugd/helpers/dev_features_chrome_remote_debugging
     and verified that EnableChromeTesting call is
     successful when ui job is restarted

Change-Id: Iaeb15ed51a614a7a8f508193f0dc79d487d378fb

[modify] https://crrev.com/8ae9ce6f6e92fd3b140219ac5684cb0c917dbcc8/debugd/src/session_manager_proxy.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/314fca4a0f457c9b88732079b951b554771f3c2d

commit 314fca4a0f457c9b88732079b951b554771f3c2d
Author: Daniel Erat <derat@chromium.org>
Date: Tue Dec 26 12:35:26 2017

autotest: Update security_SessionManagerDbusEndpoints.

Make security_SessionManagerDbusEndpoints pass an (empty)
extra_environment_variables argument when calling
session_manager's EnableChromeTesting D-Bus method. This
argument is optional but will be required in the future.

BUG= chromium:793668 
TEST=still passes when run with test_that

Change-Id: I4ac051ff82d036a336334efc48e64201a4a7ab3b

[modify] https://crrev.com/314fca4a0f457c9b88732079b951b554771f3c2d/client/site_tests/security_SessionManagerDbusEndpoints/security_SessionManagerDbusEndpoints.py

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/20b4aa3deae83d8f689859aebd27601056403c82

commit 20b4aa3deae83d8f689859aebd27601056403c82
Author: Daniel Erat <derat@chromium.org>
Date: Tue Jan 23 23:50:17 2018

[Telemetry] Pass env vars in CrOS EnableChromeTesting call.

Update cros_browser_backend's EnableChromeTesting D-Bus
method call to session_manager to pass a third argument
consisting of an (empty) array of environment variables to
set when running Chrome.

session_manager was updated to accept this arg in
https://crrev.com/c/818737, and it can be updated to require
it after Telemetry starts sending it.

Bug:  chromium:793668 
Change-Id: I522a8ca1962db22bbb39f436ca2e3d56350f1cbb
Reviewed-on: https://chromium-review.googlesource.com/877345
Commit-Queue: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>

[modify] https://crrev.com/20b4aa3deae83d8f689859aebd27601056403c82/telemetry/telemetry/internal/backends/chrome/cros_browser_backend.py

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/6429b16f46a53b81539f7e7881a3c9a8f4b01185

commit 6429b16f46a53b81539f7e7881a3c9a8f4b01185
Author: Daniel Erat <derat@chromium.org>
Date: Wed Feb 07 23:04:24 2018

login: Switch back to generated EnableChromeTesting code.

Update session_manager to use generated D-Bus bindings for
its EnableChromeTesting method. All callers have been
updated to pass the new argument containing environment
variables now, so session_manager no longer needs to use
hand-rolled code that treats the arg as optional.

BUG= chromium:793668 
TEST=updated unit tests

Change-Id: I3e56681d4de63965a4842d7808232504fa94a8c8
Reviewed-on: https://chromium-review.googlesource.com/900688
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>

[modify] https://crrev.com/6429b16f46a53b81539f7e7881a3c9a8f4b01185/login_manager/session_manager_impl.cc
[modify] https://crrev.com/6429b16f46a53b81539f7e7881a3c9a8f4b01185/login_manager/dbus_bindings/org.chromium.SessionManagerInterface.xml
[modify] https://crrev.com/6429b16f46a53b81539f7e7881a3c9a8f4b01185/login_manager/session_manager_impl_unittest.cc
[modify] https://crrev.com/6429b16f46a53b81539f7e7881a3c9a8f4b01185/login_manager/session_manager_impl.h

Comment 7 by derat@chromium.org, Feb 8 2018

Status: Verified (was: Started)

Sign in to add a comment