1155908 - [e10s] Plugin crash reports should include the content process minidump

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

Currently we get the plugin and browser process, and occasionally 3rd party flash processes, but we don't get the content process. Need to fix this for doing plugin crash/hang analysis.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

Comment 3

[Jim Mathies [:jimm]]

example: https://crash-stats.mozilla.com/report/index/d97ca6e0-b51e-4c89-a2e7-e5b052150418

Comment 4

[Benjamin Smedberg]

Will this work correctly in the non-e10s case?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Will this work correctly in the non-e10s case?

The code added is in PluginModuleChromeParent, so I think it doesn't get called. I'll check to be sure though.

Comment 6

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

Comment on attachment 8594331 [details] [diff] [review]
patch

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

Yeah, I think this will break non-e10s. PluginModuleChromeParent is used for OOP plugins in non-e10s and mContentParent will be null there. I think you just need a null check.

Also, the code is Windows-only, and I think it would be easy to generalize. CreateFlashMinidump could be renamed to CreateAdditionalPluginMinidump or something. It could use ScopedProcessHandle and stop using a DWORD for the pid. Then I think it would work on all platforms.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Bill McCloskey (:billm) from comment #6)
> Comment on attachment 8594331 [details] [diff] [review]
> patch
> 
> Review of attachment 8594331 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, I think this will break non-e10s. PluginModuleChromeParent is used for
> OOP plugins in non-e10s and mContentParent will be null there. I think you
> just need a null check.

Ah yes, the module names threw me. Added a check for mContentParent around this.

> Also, the code is Windows-only, and I think it would be easy to generalize.

Are you sure? I don't see any windows ifdef'ing around this code block.

Comment 8

[Jim Mathies [:jimm]]

> > Also, the code is Windows-only, and I think it would be easy to generalize.
> 
> Are you sure? I don't see any windows ifdef'ing around this code block.

Oh I see, MOZ_CRASHREPORTER_INJECTOR only turns on for windows via configure. Will update.

http://mxr.mozilla.org/mozilla-central/source/configure.in#6057

Comment 9

[Jim Mathies [:jimm]]

Attachment: patch v.2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2d6449911ee

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8595346 [details] [diff] [review]
patch v.2

wrong patch

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 12

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

Comment on attachment 8595350 [details] [diff] [review]
patch v.2

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +1119,5 @@
>    if (processId == 0) {
>      return false;
>    }
>  
>    base::ProcessHandle handle;

Please use ScopedProcessHandle.

@@ +1231,1 @@
>            }

Something seems wrong with the indentation here. Can you please fix?

Comment 13

[Jim Mathies [:jimm]]

Attachment: patch (r=billm)

- updated per comments

Comment 14

[Jim Mathies [:jimm]]

Attachment: patch (r=billm)

Comment 15

[Jim Mathies [:jimm]]

Attachment: patch (r=billm)

Comment 16

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5829333f12cd

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6df8807c839

Comment 18

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=9216872&repo=mozilla-inbound

Comment 19

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df3ae15819f0

Comment 20

[Jim Mathies [:jimm]]

try again, with an ifdef MOZ_CRASHREPORTER around CreatePluginMinidump:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16818c3af8f3

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c7ff528de78

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c7ff528de78

Comment 23

[Benjamin Smedberg]

I think we need to verify that it works in both e10s and non-e10s cases. Jim, who's the right person for that?

Comment 24

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #23)
> I think we need to verify that it works in both e10s and non-e10s cases.
> Jim, who's the right person for that?

I've verified reports are showing up for both in the 24th build.