Problematic use of OFFICIAL_BUILD to customize crash reporting behavior

[Marshall Greenblatt]

Hi John,

In https://codereview.chromium.org/1291553003 you changed the crash reporting behavior so that stack traces are printed to console in non-official release builds, introducing the use of OFFICIAL_BUILD in a number of new locations. For example, your changes added the following code in content/app/content_main_runner.cc:

#if !defined(OFFICIAL_BUILD)
  // Print stack traces to stderr when crashes occur. This opens up security
  // holes so it should never be enabled for official builds.
  base::debug::EnableInProcessStackDumping();
#if defined(OS_WIN)
  base::RouteStdioToConsole(false);
  LoadLibraryA("dbghelp.dll");
#endif
#endif

This specific change is problematic because:

1. Other release-quality applications based on the Content API do not define OFFICIAL_BUILD. Consequently this insecure behavior is enabled for those applications.

2. This change introduces a runtime dependency on dbghelp.dll which is not included by default on Windows XP systems. Consequently all XP installs must now bundle dbghelp.dll.

I would like to propose that we not use OFFICIAL_BUILD to customize the crash reporting behavior but instead provide a finer-grained toggle that other Content API consumers can selectively enable or disable. WDYT?

Thanks,

Marshall

[Torne (Richard Coles)]

Not saying this specific change is a good idea or not, but more generally:

I think anyone shipping apps based on Chromium to users *should* be defining OFFICIAL_BUILD, and if there's any problem preventing them from doing so then we should fix those problems. OFFICIAL_BUILD is intended to be the final "this is for actual users to use" switch, vs building in Release which is "don't include debug stuff because I want to have reasonably realistic performance/memory numbers".

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Marshall Greenblatt]

On Fri, Dec 18, 2015 at 12:06 PM, Torne (Richard Coles) <to...@chromium.org> wrote:

Not saying this specific change is a good idea or not, but more generally:

I think anyone shipping apps based on Chromium to users *should* be defining OFFICIAL_BUILD, and if there's any problem preventing them from doing so then we should fix those problems. OFFICIAL_BUILD is intended to be the final "this is for actual users to use" switch, vs building in Release which is "don't include debug stuff because I want to have reasonably realistic performance/memory numbers".

OK, I'm open to that idea :). The biggest problem seems to be conflation of OFFICIAL_BUILD and GOOGLE_CHROME_BUILD. For example, [1] and [2]. There are a lot of places using this define so it's a bit hard to audit effectively. I'll try building my application with OFFICIAL_BUILD and see if anything breaks.

[1] https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/branding.cc&l=12

[2] https://code.google.com/p/chromium/codesearch#chromium/src/components/os_crypt/keychain_password_mac.mm&l=54

[Torne (Richard Coles)]

Yeah, those are incorrect and should be testing for the Chrome branding instead (or maybe the AND of both). WebView builds its non-branded AOSP binaries with OFFICIAL_BUILD (and there are probably other examples) so it's not even only third parties who would have issues with these.

[Scott Graham]

1 & 2 are definitely wrong and should be fixed in any case.

[Marshall Greenblatt]

Thanks, I've filed http://crbug.com/574126. Building with GYP_DEFINES=buildtype=Official seems to work for my application. I'll use that ticket to track any additional problems that I run into related to this.

[John Abd-El-Malek]

Agreed that anyone shipping builds should be setting OFFICIAL_BUILD.

Regarding XP, we still want to have this code run for non-official XP because otherwise main waterfall crashes in any of our test suites that are XP-specific (which do happen) don't give meaningful stack traces. This makes debugging really hard, as we still don't have XP trybots, and getting access to an XP VM is non-trivial.

Regarding using another flag, I think this isn't necessary for the reason above (and given by others): that other content embedders should be setting OFFICIAL_BUILD for builds that they release.