Closed Bug 1278325 Opened 3 years ago Closed 3 years ago

Hook exit to report actual crash for better information in crash reports

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: ernest, Assigned: ernest)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Third party code can call exit on a crash, leading to a crash reported due to static destructor of service being run off main thread. Want to hook exit to end crash report at the third party code to properly report reason for crash, and not report the bad state crash due to the actual third party crash.
Also we want to prevent 3rd party code from asserting and calling exit() like the AppleIntel driver does and not getting a crash from that abnormal shutdown.
Core:Graphics?  Seems like a more widespread thing.
Component: Graphics → General
Comment on attachment 8760902 [details]
Bug 1278325 - Hook exit to report actual crash for better information in crash reports

https://reviewboard.mozilla.org/r/58296/#review55180

::: toolkit/xre/nsAppRunner.h:129
(Diff revision 1)
>  
>  /**
> + * Runs MOZ_CRASH if actualCrashFlag in crashFlag.h is not set to true
> + * Used to differentiate third party exit calls from actual exit calls from our code
> + */
> +void runMozCrash();

This needs a better name. The name sounds like it's just a wrapper around MozCrash. How about:
AbnormalShutdownHook. Then you can rename the flag to gNormalShutdown (it's a bit of a negative but we save a 'true' init which means it's can be inited by the .bss section.

The block comment needs to explain the why a bit better:
Shutdown hook to prevent 3rd party code, like the Apple Intel Driver, from triggering an uncaught unexpected shutdown. We'd rather get a crash to get this data into socoro. As well we only support a full shutdown if it's on the main thread and initiated by Gecko.

::: toolkit/xre/nsNativeAppSupportUnix.cpp:28
(Diff revision 1)
>  #include "nsIBaseWindow.h"
>  #include "nsIWidget.h"
>  #include "nsIWritablePropertyBag2.h"
>  #include "nsIPrefService.h"
>  #include "mozilla/Services.h"
> +#include "crashFlag.h"

forget hg add? Also it's not worth adding a file for just one flag. Find an appropriate place (something that deals with shutdown) in XRE to add the flag.
Attachment #8760902 - Flags: review?(bgirard)
Tested flag to make sure it works on exit calls without flag set. Currently trying to check to see if crashreporter (or breakpad) exit calls are affected by this change (but probably should spawn separate process?)
Flags: needinfo?(bgirard)
I'm not sure what we want to do here. We're calling exit() somewhere in the crashreporter code? We don't seem to do that in nsExceptionHandler.cpp:
https://dxr.mozilla.org/mozilla-central/rev/cf06fbc831754e54c6abb71d3136597488a530e0/toolkit/crashreporter/nsExceptionHandler.cpp#1082

Is it in Breakpad somewhere? Would it help if we called _exit() after posix_spawnp instead?
Comment on attachment 8782154 [details]
Bug 1278325 - Hook exit to catch early 3rd party exits in crash reports.

https://reviewboard.mozilla.org/r/72312/#review71454

I'm clearing this review until we get that sorted out.
Attachment #8782154 - Flags: review?(ted)
We chatted on IRC that I was just adding this check in case to prevent accidental re-entrancy even though I don't think it is a problem.
Attachment #8760902 - Attachment is obsolete: true
Flags: needinfo?(bgirard)
Attachment #8782154 - Flags: review?(ted)
Comment on attachment 8782154 [details]
Bug 1278325 - Hook exit to catch early 3rd party exits in crash reports.

https://reviewboard.mozilla.org/r/72312/#review77320

::: toolkit/xre/nsAppRunner.cpp:388
(Diff revision 2)
> +  gIsExpectedExit = true;
> +}
> +
> +/**
> + * Runs atexit() to catch unexpected exit from 3rd party libraries. When they
> + * call exit() to report an error we wont shutdown correctly and wont catch

spelling nit: `won't`.

You might want to mention the context here--that some third-party code like drivers attempts to handle crashes by calling `exit`.

::: toolkit/xre/nsAppRunner.cpp:3023
(Diff revision 2)
>    if (!aExitFlag)
>      return 1;
>    *aExitFlag = false;
>  
> +  atexit(UnexpectedExit);
> +  auto expectedShutdown = mozilla::MakeScopeExit([&] {

That is some fancy modern C++!
Attachment #8782154 - Flags: review?(ted) → review+
Pushed by b56girard@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a42e8a53dc39
Hook exit to catch early 3rd party exits in crash reports. r=ted
https://hg.mozilla.org/mozilla-central/rev/a42e8a53dc39
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.