816963 - Enable crash reporting in places C++ unit tests

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[(not currently active) Ted Mielczarek]

The new harness from bug 787176 supports processing minidumps from C++ unit test crashes, but due to the inconsistent state of our C++ unit tests crash reporting isn't actually enabled globally. This patch makes the places C++ unit tests enable crash reporting so we can get useful stack traces out of test_IHistory crashes.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Enable crash reporting in places C++ unit tests

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 687072 [details] [diff] [review]
Enable crash reporting in places C++ unit tests

Review of attachment 687072 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/cpp/places_test_harness_tail.h
@@ +97,5 @@
> +#ifdef MOZ_CRASHREPORTER
> +    char* enabled = PR_GetEnv("MOZ_CRASHREPORTER");
> +    if (enabled && !strcmp(enabled, "1")) {
> +      //TODO: move this to an even-more-common location to use in all
> +      // C++ unittests

Is there a bug filed for this? would be nice to replace "TODO:" with "Bug XYZ:" (nit: space after //)

@@ +104,5 @@
> +      if (crashreporter) {
> +        fprintf(stderr, "Setting up crash reporting\n");
> +
> +        nsCOMPtr<nsIProperties> dirsvc =
> +            do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID);

wrong indentation (just 2 spaces please)

also missing null check for dirsvc

@@ +109,5 @@
> +        nsCOMPtr<nsIFile> cwd;
> +        nsresult rv = dirsvc->Get(NS_OS_CURRENT_WORKING_DIR,
> +                                  NS_GET_IID(nsIFile),
> +                                  getter_AddRefs(cwd));
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

I think these tests also run in opt mode, we should not use the output value if there's a failure.

In both checks you may probably use NS_RUNTIMEABORT, they should actually never happen, but better being overzealous.

Comment 3

[(not currently active) Ted Mielczarek]

(In reply to Marco Bonardo [:mak] from comment #2)
> > +      //TODO: move this to an even-more-common location to use in all
> > +      // C++ unittests
> 
> Is there a bug filed for this? would be nice to replace "TODO:" with "Bug
> XYZ:" (nit: space after //)

Yes, this is bug 787458, I put it in the comment. Fixed the rest of your comments as well, thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc142852060

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8fc142852060