Switch PPluginModule away from PCrashReporter

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Similar to bug 1337518. Goal is to remove PCrashReporter and switch to shmem-based metadata.
Created attachment 8835656 [details] [diff] [review]
part 1, move extra notes

Store extra notes on CrashReporterHost, instead of passing them into the GenerateCrashReport function.
Attachment #8835656 - Flags: review?(wmccloskey)
Created attachment 8835657 [details] [diff] [review]
part 2, store ThreadId

Store the NativeThreadId on CrashReporterHost. This will only be used for hang reporting.
Attachment #8835657 - Flags: review?(wmccloskey)
Created attachment 8835659 [details] [diff] [review]
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.
Attachment #8835659 - Flags: review?(wmccloskey)
Created attachment 8835661 [details] [diff] [review]
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.
Attachment #8835661 - Flags: review?(wmccloskey)
Created attachment 8835665 [details] [diff] [review]
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.
Attachment #8835665 - Flags: review?(wmccloskey)
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.
Attachment #8835665 - Flags: review?(wmccloskey)
Attachment #8835656 - Flags: review?(wmccloskey) → review+
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.
Attachment #8835657 - Flags: review?(wmccloskey) → review+
Attachment #8835659 - Flags: review?(wmccloskey) → review+
Attachment #8835661 - Flags: review?(wmccloskey) → review+
Created attachment 8836503 [details] [diff] [review]
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.
Attachment #8835665 - Attachment is obsolete: true
Attachment #8836503 - Flags: review?(wmccloskey)
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
Attachment #8836503 - Flags: review?(wmccloskey) → review+

Comment 10

2 years ago
Pushed by danderson@mozilla.com:
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)
Please request Aurora approval on this when you feel it's sufficiently-baked.
status-firefox53: --- → affected
Flags: needinfo?(dvander)
(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.
(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

2 years ago
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.
Flags: needinfo?(dvander) → needinfo?(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.
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.