1338281 - Switch PPluginModule away from PCrashReporter

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Similar to bug 1337518. Goal is to remove PCrashReporter and switch to shmem-based metadata.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, move extra notes

Store extra notes on CrashReporterHost, instead of passing them into the GenerateCrashReport function.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, store ThreadId

Store the NativeThreadId on CrashReporterHost. This will only be used for hang reporting.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, init shmem from parent

This allows initializing the metadata shmem from the CrashReporterHost instead of client. I didn't want to change the plugin code too much and it uses a parent->child interrupt to start the crash reporter.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, refactor

This refactors CrashReporterHost, splitting the Generate function into separate "take minidump" and "send minidump" steps. It now also stores the crash dump ID so PluginModuleParent won't have to.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, rm PCrashReporter use

This should be functionally equivalent to before. For simplicity I removed a bunch of XP_WIN stuff around the hang reporter mutex.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8835665 [details] [diff] [review]
part 5, rm PCrashReporter use

This is failing some tests on try, will re-r? when it looks good.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8835657 [details] [diff] [review]
part 2, store ThreadId

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

::: ipc/glue/CrashReporterClient.h
@@ +11,5 @@
>  #include "mozilla/StaticPtr.h"
>  #include "mozilla/Unused.h"
>  #include "mozilla/ipc/Shmem.h"
>  
> +#ifdef MOZ_CRASHREPORTER 

Extra space at the end.

@@ +73,5 @@
>  };
>  
>  } // namespace ipc
>  } // namespace mozilla
> +#endif // MOZ_CRASHREPORTER 

Same.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, switch to shmem

Switch from PCrashReporter to shmem. This one seems to pass tests, which luckily rooted out a bunch of subtle bugs.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8836503 [details] [diff] [review]
part 5, switch to shmem

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

::: dom/plugins/ipc/PluginModuleParent.h
@@ +362,5 @@
> +#ifdef MOZ_CRASHREPORTER
> +    /**
> +     * This mutex protects the crash reporter when the Plugin Hang UI event
> +     * handler is executing off main thread. It is intended to protect both
> +     * the mCrashReporter variable in addition to the CrashReporterParent object

CrashReporterHost

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7838f017fe4c
Store extra annotations directly on CrashReporterHost. (bug 1338281 part 1, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8622ecc4e6
Store the child process thread id in CrashReporterHost. (bug 1338281 part 2, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5c80258cb2
Allow initializing CrashReporterClient shmem through CrashReporterHost. (bug 1338281 part 3, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7adc2bbf8e
Allow finalizing external crash reports from CrashReportHost. (bug 1338281 part 4, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e78a0da7268
Switch PPluginModule from PCrashReporter to shmem-based CrashReportHost/Client. (bug 1338281 part 5, r=billm)

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7838f017fe4c
https://hg.mozilla.org/mozilla-central/rev/3f8622ecc4e6
https://hg.mozilla.org/mozilla-central/rev/5d5c80258cb2
https://hg.mozilla.org/mozilla-central/rev/0e7adc2bbf8e
https://hg.mozilla.org/mozilla-central/rev/2e78a0da7268

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you feel it's sufficiently-baked.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Please request Aurora approval on this when you feel it's sufficiently-baked.

If this is to fix bug 1339338 - we'd need this bug, bug 1338308, and bug 1338281. It's on the invasive side for uplifting. But if we really want it, I'll request uplift in a few days.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to David Anderson [:dvander] from comment #13)
> we'd need this bug, bug 1338308, and bug 1338281

Did you mean another bug here rather than 1338281? Since that's this one :)

Comment 15

[Benjamin Smedberg]

Ryan, is there a particular reason you want to uplift this? Given the risk of breaking crash-stats or telemetry, this doesn't look like a good candidate for uplift.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Because the crashes in bug 1339338 affect 53 as well. I'm open to a more branch-friendly option for uplift or wontfixing if need-be though.