1115438 - Toolhelp32 snapshot janking plugin module init

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

On my 2012-vintage ThinkPad this causes about 35-40ms of jank. Can we move the call to CreateToolhelp32Shapsot off the main thread? I know that this would race with the plugin container but I don't think we do anything to prevent that right now anyway.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

dmajor, bsmedberg: Do you have any comments or concerns re: this bug?

Comment 2

[(Away)]

I notice that we leak the snapshot handle :) Other than that I'm not super familiar with this code. Bsmedberg would know more about how long we can safely delay this stuff.

Kinda sad that we have to enumerate all system processes in order to look up some children. I don't know of a better way though.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to David Major [:dmajor] (UTC+13) from comment #2)
> I notice that we leak the snapshot handle :)
Filed bug 1118031. Nice catch!

> Kinda sad that we have to enumerate all system processes in order to look up
> some children. I don't know of a better way though.
Me neither. I went digging to see if there were other APIs that could narrow the scope but from what I could see the alternatives weren't any better. :(

Comment 4

[Benjamin Smedberg]

This is the crash injector code? I don't think it would be a big deal to make that asynchronous, as long as `InjectCrashReporterIntoProcess` happens on the main thread.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Wrote a patch but I need to retest with my fix for bug 1119060.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Move toolhelp32 snapshot off the main thread

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8546278 [details] [diff] [review]
Move toolhelp32 snapshot off the main thread

Georg hasn't been able to get to this and is now on vacation.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8546278 [details] [diff] [review]
Move toolhelp32 snapshot off the main thread

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

Generally looks good. I'm a little wary of the use of QueueUserWorkItem for the first time, but I imagine anything negative from that will have ample time to show up in testing.

Just out of curiosity, have you tested any of this flash process injection code with e10s recently? We have bug 1096080 filed indicating it doesn't work but I haven't tested that in a while.

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +106,5 @@
>      return PPluginModule::Bridge(aContentParent, chromeParent);
>  }
>  
>  /**
> + * Use for executing CreateToolhelp32Snapshot off main thread (bug 1115438)

nit - no need for bug number comments about resolved bugs. blame takes care of that.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #8)
> Comment on attachment 8546278 [details] [diff] [review]
> Move toolhelp32 snapshot off the main thread
> 
> Review of attachment 8546278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Generally looks good. I'm a little wary of the use of QueueUserWorkItem for
> the first time, but I imagine anything negative from that will have ample
> time to show up in testing.

I've used it extensively in another code base so I'm not too concerned.

> 
> Just out of curiosity, have you tested any of this flash process injection
> code with e10s recently? We have bug 1096080 filed indicating it doesn't
> work but I haven't tested that in a while.

No idea. :-/

> 
> ::: dom/plugins/ipc/PluginModuleParent.cpp
> @@ +106,5 @@
> >      return PPluginModule::Bridge(aContentParent, chromeParent);
> >  }
> >  
> >  /**
> > + * Use for executing CreateToolhelp32Snapshot off main thread (bug 1115438)
> 
> nit - no need for bug number comments about resolved bugs. blame takes care
> of that.

Will do.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Move toolhelp32 snapshot off the main thread (r2)

Addressed comment and fixed merge conflicts. Carrying forward r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2812517c3814

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/003cac9e4e67 for being a little lacking in the "actually compiles" department, https://treeherder.mozilla.org/logviewer.html#?job_id=7279680&repo=mozilla-inbound or https://treeherder.mozilla.org/logviewer.html#?job_id=7279670&repo=mozilla-inbound or https://treeherder.mozilla.org/logviewer.html#?job_id=7279675&repo=mozilla-inbound depending on which compiler's messages you like best.

Comment 12

[Jim Mathies [:jimm]]

> > Just out of curiosity, have you tested any of this flash process injection
> > code with e10s recently? We have bug 1096080 filed indicating it doesn't
> > work but I haven't tested that in a while.
> 
> No idea. :-/

Resolved wfm the other day, appears to be working.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Move toolhelp32 snapshot off the main thread (r3)

Missed an #ifdef for MOZ_CRASHREPORTER_INJECTOR, so only Win32 built.

https://hg.mozilla.org/integration/mozilla-inbound/rev/717f0399930e

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/717f0399930e