Closed Bug 816963 Opened 7 years ago Closed 7 years ago

Enable crash reporting in places C++ unit tests

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(1 file)

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.
Blocks: 673017
Status: NEW → ASSIGNED
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.
Attachment #687072 - Flags: review?(mak77) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/8fc142852060
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.