1105039 - Gecko profiler uses intr messages from chrome to content process

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

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

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#518

Mike, this is something we really want to avoid. Is there any reason we need to use intr here?

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I was following the pattern that I saw pre-existing in PPluginModule.ipdl[1].

I believe the Gecko Profiler, when gathering profiles, assumes it will receive those profiles synchronously. BenWa can confirm or refute my read on that. If I'm wrong, we could certainly switch this to an async call.

Otherwise, is there something synchronous other than intr that we should use here?

[1]: http://hg.mozilla.org/mozilla-central/file/8c37c5083952/dom/plugins/ipc/PPluginModule.ipdl#l84

Comment 2

[Benoit Girard (:BenWa)]

This used to be RPC but got changed here:
http://hg.mozilla.org/mozilla-central/diff/2466893f18a7/dom/plugins/ipc/PPluginModule.ipdl

TBH I don't know why I made it RPC. I think it should of just been Sync all along. The reason this is async is because nsProfiler must return the profile synchronously.

I vote to change both to 'sync'.

Comment 3

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

We simply can't make it sync. Sync messages from the chrome process are forbidden.

Benoit, can you lay out a plan to change nsProfiler so this doesn't have to be synchronous?

Comment 4

[Benoit Girard (:BenWa)]

Shouldn't be too hard it looks like. I think we just have to change the addon (simple) and change devtools.

Victor how big is the change for devtools to get the profile asynchronously? Looks like the profiler actor is already async anyways so it should be trivial right?

Comment 5

[Victor Porof [:vporof][:vp]]

The profiler frontend already expects everything to be async. The backend (actor) expects the profiler module to start synchronously at the moment [0]. Changing this is quite possibly trivial, and IIRC it involves making `onStartProfiler` return a promise (although I can't exactly remember if old-style actors work like this or some other similar way).

[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/profiler.js?from=actors/profiler.js#58

Comment 6

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

Attachment: profiler-cpow

I want to get this out of the way so we can finally be rid of intr messages. The easiest way is to switch to a high priority message, like CPOWs use. It's still sync and it avoids many of the problems with intr messages.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8535945 [details] [diff] [review]
profiler-cpow

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

If you're changing this one, can you change the GetProfile call in PPluginModule.ipdl too?

Comment 8

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

Comment on attachment 8535945 [details] [diff] [review]
profiler-cpow

The basic problem we have right now is that intr messages and CPOW messages (i.e., prioritized sync messages) don't work together. If there's a protocol that uses both, it's probably broken.

Since PContent has to use CPOWs, we want to get rid of all the intr messages. Since PPluginModule has to use intr messages, we don't want to introduce any CPOW messages.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8535945 [details] [diff] [review]
profiler-cpow

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

Ah OK - I was not aware of that restriction. Thanks for clearing that up.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/81a0a75573ea

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81a0a75573ea