Restrict new crash_reporter flag to valid versions [crashpad/crashpad : main]

4 views
Skip to first unread message

Miriam Zimmerman (Gerrit)

unread,
Mar 3, 2023, 12:14:33 PM3/3/23
to Mark Mentovai, Hidehiko Abe, crashp...@chromium.org

Attention is currently required from: Hidehiko Abe, Mark Mentovai.

View Change

    To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: crashpad/crashpad
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
    Gerrit-Change-Number: 4308129
    Gerrit-PatchSet: 1
    Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
    Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
    Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
    Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
    Gerrit-Attention: Mark Mentovai <ma...@chromium.org>
    Gerrit-Comment-Date: Fri, 03 Mar 2023 17:14:30 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: No
    Gerrit-MessageType: comment

    Mark Mentovai (Gerrit)

    unread,
    Mar 3, 2023, 12:19:29 PM3/3/23
    to Miriam Zimmerman, Hidehiko Abe, crashp...@chromium.org

    Attention is currently required from: Hidehiko Abe, Miriam Zimmerman.

    Patch set 1:Code-Review +1

    View Change

    1 comment:

    • File handler/linux/cros_crash_report_exception_handler.cc:

      • Patch Set #1, Line 255: constexpr int32_t kFixedVersion = 15363;

        This number doesn't mean anything to me, so I trust that you've gotten it right.

    To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: crashpad/crashpad
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
    Gerrit-Change-Number: 4308129
    Gerrit-PatchSet: 1
    Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
    Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
    Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
    Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
    Gerrit-Attention: Miriam Zimmerman <mute...@chromium.org>
    Gerrit-Comment-Date: Fri, 03 Mar 2023 17:19:25 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Gerrit-MessageType: comment

    Miriam Zimmerman (Gerrit)

    unread,
    Mar 3, 2023, 12:20:30 PM3/3/23
    to Mark Mentovai, Hidehiko Abe, crashp...@chromium.org

    Attention is currently required from: Hidehiko Abe.

    View Change

    2 comments:

    • Patchset:

    • File handler/linux/cros_crash_report_exception_handler.cc:

      • Patch Set #1, Line 255: constexpr int32_t kFixedVersion = 15363;

        This number doesn't mean anything to me, so I trust that you've gotten it right.

    To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: crashpad/crashpad
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
    Gerrit-Change-Number: 4308129
    Gerrit-PatchSet: 1
    Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
    Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
    Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
    Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
    Gerrit-Comment-Date: Fri, 03 Mar 2023 17:20:27 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Mark Mentovai <ma...@chromium.org>
    Gerrit-MessageType: comment

    Miriam Zimmerman (Gerrit)

    unread,
    Mar 3, 2023, 12:21:11 PM3/3/23
    to Mark Mentovai, Hidehiko Abe, crashp...@chromium.org

    Attention is currently required from: Hidehiko Abe.

    Patch set 1:Commit-Queue +1

    View Change

      To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: crashpad/crashpad
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
      Gerrit-Change-Number: 4308129
      Gerrit-PatchSet: 1
      Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
      Gerrit-Reviewer: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Comment-Date: Fri, 03 Mar 2023 17:21:08 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      Gerrit-MessageType: comment

      Miriam Zimmerman (Gerrit)

      unread,
      Mar 3, 2023, 12:32:40 PM3/3/23
      to Crashpad LUCI CQ, Mark Mentovai, Hidehiko Abe, crashp...@chromium.org

      Attention is currently required from: Hidehiko Abe.

      View Change

      1 comment:

      • Patchset:

        • Patch Set #1:

          this change breaks because mini_chromium doesn't include base/system/sys_info.h (or any of base/system)

      To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: crashpad/crashpad
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
      Gerrit-Change-Number: 4308129
      Gerrit-PatchSet: 1
      Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
      Gerrit-Reviewer: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Comment-Date: Fri, 03 Mar 2023 17:32:37 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Gerrit-MessageType: comment

      Mark Mentovai (Gerrit)

      unread,
      Mar 3, 2023, 1:00:35 PM3/3/23
      to Miriam Zimmerman, Crashpad LUCI CQ, Hidehiko Abe, crashp...@chromium.org

      Attention is currently required from: Hidehiko Abe, Miriam Zimmerman.

      View Change

      3 comments:

      • Patchset:

      • File handler/linux/cros_crash_report_exception_handler.cc:

        • Patch Set #1, Line 21: #include "base/system/sys_info.h"

          You can wrap this in `#if defined(CRASHPAD_IS_IN_CHROMIUM)`.

        • Patch Set #1, Line 251: int32_t major_version = 0, minor_version = 0, bugfix_version = 0;

          And do something similar here:

          ```
          #if defined(CRASHPAD_IS_IN_CHROMIUM)
          int32_t major_version = …
          const bool use_chrome_signal_argument = major_version >= kFixedVersion;
          #else
          cosnt bool use_chrome_signal_argument = true;
          #endif
          if (use_chrome_signal_argument) {
          const ExceptionShapshot* const exception_snapshot = …
          }
          ```

      To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: crashpad/crashpad
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
      Gerrit-Change-Number: 4308129
      Gerrit-PatchSet: 1
      Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
      Gerrit-Reviewer: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Attention: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Comment-Date: Fri, 03 Mar 2023 18:00:31 +0000

      Miriam Zimmerman (Gerrit)

      unread,
      Mar 3, 2023, 1:21:38 PM3/3/23
      to Crashpad LUCI CQ, Mark Mentovai, Hidehiko Abe, crashp...@chromium.org

      Attention is currently required from: Hidehiko Abe.

      Patch set 2:Commit-Queue +1

      View Change

      2 comments:

      • File handler/linux/cros_crash_report_exception_handler.cc:

        • Patch Set #1, Line 21: #include "base/system/sys_info.h"

          You can wrap this in `#if defined(CRASHPAD_IS_IN_CHROMIUM)`.

        • Done

        • And do something similar here: […]

          Falling back to a default of *false* just in case there's unexpected version skew with crashpad *not* in chromium.

      To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: crashpad/crashpad
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
      Gerrit-Change-Number: 4308129
      Gerrit-PatchSet: 2
      Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
      Gerrit-Reviewer: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Comment-Date: Fri, 03 Mar 2023 18:21:35 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes

      Miriam Zimmerman (Gerrit)

      unread,
      Mar 3, 2023, 1:22:14 PM3/3/23
      to Crashpad LUCI CQ, Mark Mentovai, Hidehiko Abe, crashp...@chromium.org

      Attention is currently required from: Hidehiko Abe.

      View Change

      1 comment:

      • Patchset:

        • Patch Set #1:

          this change breaks because mini_chromium doesn't include base/system/sys_info. […]

          Done

      To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: crashpad/crashpad
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
      Gerrit-Change-Number: 4308129
      Gerrit-PatchSet: 2
      Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
      Gerrit-Reviewer: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Comment-Date: Fri, 03 Mar 2023 18:22:12 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-MessageType: comment

      Mark Mentovai (Gerrit)

      unread,
      Mar 3, 2023, 1:31:19 PM3/3/23
      to Miriam Zimmerman, Crashpad LUCI CQ, Hidehiko Abe, crashp...@chromium.org

      Attention is currently required from: Hidehiko Abe, Miriam Zimmerman.

      Patch set 2:Code-Review +1

      View Change

      1 comment:

      • File handler/linux/cros_crash_report_exception_handler.cc:

        • Patch Set #2, Line 21: #if defined(CRASHPAD_IS_IN_CHROMIUM)

          One more thing I forgot to mention: in handler/BUILD.gn, you need to add config to get this set.

          At:

          ```
          static_library("handler") {
          […]
          if (crashpad_is_linux) {
          sources += [
          "linux/cros_crash_report_exception_handler.cc",
          "linux/cros_crash_report_exception_handler.h",
          ]
          }
          }
          ```

          Introduce this config alongside the `sources`:


          ```
          configs += [
          "../build:crashpad_is_in_chromium",
          ]
          ```

          along with a TODO to remove it in the future, just like the TODO in this file.

      To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: crashpad/crashpad
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
      Gerrit-Change-Number: 4308129
      Gerrit-PatchSet: 2
      Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
      Gerrit-Reviewer: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Attention: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Comment-Date: Fri, 03 Mar 2023 18:31:16 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      Gerrit-MessageType: comment

      Miriam Zimmerman (Gerrit)

      unread,
      Mar 3, 2023, 1:34:48 PM3/3/23
      to Crashpad LUCI CQ, Mark Mentovai, Hidehiko Abe, crashp...@chromium.org

      Attention is currently required from: Hidehiko Abe.

      Patch set 3:Commit-Queue +1

      View Change

      1 comment:

      • File handler/linux/cros_crash_report_exception_handler.cc:

        • One more thing I forgot to mention: in handler/BUILD.gn, you need to add config to get this set. […]

          Done

      To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: crashpad/crashpad
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
      Gerrit-Change-Number: 4308129
      Gerrit-PatchSet: 3
      Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
      Gerrit-Reviewer: Miriam Zimmerman <mute...@chromium.org>
      Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
      Gerrit-Comment-Date: Fri, 03 Mar 2023 18:34:44 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes

      Mark Mentovai (Gerrit)

      unread,
      Mar 3, 2023, 1:47:39 PM3/3/23
      to Miriam Zimmerman, Crashpad LUCI CQ, Hidehiko Abe, crashp...@chromium.org

      Attention is currently required from: Hidehiko Abe, Miriam Zimmerman.

      Patch set 3:Code-Review +1Commit-Queue +2

      View Change

        To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-Project: crashpad/crashpad
        Gerrit-Branch: main
        Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
        Gerrit-Change-Number: 4308129
        Gerrit-PatchSet: 3
        Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
        Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
        Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
        Gerrit-Reviewer: Miriam Zimmerman <mute...@chromium.org>
        Gerrit-Attention: Hidehiko Abe <hide...@chromium.org>
        Gerrit-Attention: Miriam Zimmerman <mute...@chromium.org>
        Gerrit-Comment-Date: Fri, 03 Mar 2023 18:47:35 +0000

        Crashpad LUCI CQ (Gerrit)

        unread,
        Mar 3, 2023, 1:47:46 PM3/3/23
        to Miriam Zimmerman, Mark Mentovai, Hidehiko Abe, crashp...@chromium.org

        Crashpad LUCI CQ submitted this change.

        View Change

        Approvals: Mark Mentovai: Looks good to me; Commit
        Restrict new crash_reporter flag to valid versions

        Lacros can be up to 2 milestones ahead of ash (and consequently the
        platform code), so until the crash_reporter change has been in for 2
        milestones, we need to manually check version compatibility.

        BUG=chromium:1420445
        TEST=Build, deploy, check that flag is set only on right version

        Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
        Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4308129
        Commit-Queue: Mark Mentovai <ma...@chromium.org>
        Reviewed-by: Mark Mentovai <ma...@chromium.org>
        ---
        M handler/BUILD.gn
        M handler/linux/cros_crash_report_exception_handler.cc
        2 files changed, 28 insertions(+), 8 deletions(-)


        To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-Project: crashpad/crashpad
        Gerrit-Branch: main
        Gerrit-Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
        Gerrit-Change-Number: 4308129
        Gerrit-PatchSet: 4
        Gerrit-Owner: Miriam Zimmerman <mute...@chromium.org>
        Gerrit-Reviewer: Crashpad LUCI CQ <crashpa...@luci-project-accounts.iam.gserviceaccount.com>
        Gerrit-Reviewer: Hidehiko Abe <hide...@chromium.org>
        Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
        Gerrit-Reviewer: Miriam Zimmerman <mute...@chromium.org>
        Gerrit-MessageType: merged
        Reply all
        Reply to author
        Forward
        0 new messages