Closed Bug 1268938 Opened 4 years ago Closed 4 years ago

Most reply Messages don't have a name set

Categories

(Core :: IPC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s + ---
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 1 obsolete file)

Bug 1264820 adds telemetry for the size of reply messages. However, most, but not all, of them seem to not set a name, so the name is just "???". It seems like sync calls that don't return values, like PCompositorBridge::FlushRendering, set the name, but those that do, like PContent::GetXPCOMProcessAttributes do not. This also shows up in existing IPC logging.
Whiteboard: btpp-active
I guess what happens is that when you send a message, it goes like IPDL -> MessageChannel -> Chromium IPC. IPDL sets the name of the message, and telemetry reports the name. When you get a reply, it goes in the opposite direction, so when you are doing telemetry in MessageChannel before the name is set. Actually, nothing sets the message name in IPDL for a reply, but it could.

I'm not 100% sure why some messages have their name set, but I think they are not coming directly from the other process. (They are coming in via ThreadLink::SendMessage() instead of ProcessLink::OnMessageReceived().)
Summary: Sync messages that return a value don't set the message name → Most reply Messages don't have a name set
This makes Send() and Call() take an extra argument that is the message name. Before the methods return the reply message or do telemetry, set the message. All callers of Send() and Call() are in codegenned IPDL code, so they are easy to fix, using prettyReplyName.

Also, we don't collect telemetry for the reply messages in Call(), but I didn't see any when loading cnn.com so hopefully they aren't too important. I could also just add telemetry for that. I'm not sure what it is for.
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Also, we don't collect telemetry for the reply messages in Call(), but I
> didn't see any when loading cnn.com so hopefully they aren't too important.

It looks like Call is only for `intr` messages, and it looks like those are used only by plugins (heavily by NPAPI and a few things in GMP).
Attachment #8747312 - Flags: review?(wmccloskey)
Comment on attachment 8747312 [details] [diff] [review]
Set a name for reply messages.

Bill pointed out that we could just take the name from the sending message and use that, rather than pass a new thing around.
Attachment #8747312 - Flags: review?(wmccloskey)
This is much nicer than my original patch.

As I mentioned to Bill, the root of this is that there was a bug in my fix for the bug in Bill's original patch in bug 1264820.
Attachment #8748001 - Flags: review?(wmccloskey)
Attachment #8747312 - Attachment is obsolete: true
Attachment #8748001 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/53d3b7cdb82f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1264820
See Also: 1264820
Comment on attachment 8748001 [details] [diff] [review]
Use the name of the original message in Send for reply telemetry.

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: This improves some telemetry that might help investigating e10s OOM crashes.
[Describe test coverage new/current, TreeHerder]: this code runs very frequently.
[Risks and why]: Low. The patch is simple. I verified that the expected telemetry has started showing up in Nightly.
[String/UUID change made/needed]: none
Attachment #8748001 - Flags: approval-mozilla-beta?
Attachment #8748001 - Flags: approval-mozilla-aurora?
Comment on attachment 8748001 [details] [diff] [review]
Use the name of the original message in Send for reply telemetry.

Needed to better diagnose e10s OOM crashes, telemetry data was verified on Nightly, Aurora48+, Beta47+
Attachment #8748001 - Flags: approval-mozilla-beta?
Attachment #8748001 - Flags: approval-mozilla-beta+
Attachment #8748001 - Flags: approval-mozilla-aurora?
Attachment #8748001 - Flags: approval-mozilla-aurora+
tracking-e10s: --- → +
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.