Closed Bug 1338281 Opened 4 years ago Closed 4 years ago

Switch PPluginModule away from PCrashReporter

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(5 files, 1 obsolete file)

Similar to bug 1337518. Goal is to remove PCrashReporter and switch to shmem-based metadata.
Store extra notes on CrashReporterHost, instead of passing them into the GenerateCrashReport function.
Attachment #8835656 - Flags: review?(wmccloskey)
Store the NativeThreadId on CrashReporterHost. This will only be used for hang reporting.
Attachment #8835657 - Flags: review?(wmccloskey)
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)
Attached patch part 4, refactorSplinter Review
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)
Attached patch part 5, rm PCrashReporter use (obsolete) — Splinter Review
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+
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+
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.
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 :)
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.