Most reply Messages don't have a name set

RESOLVED FIXED in Firefox 47

Status

()

Core
IPC
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 fixed, firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
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
(Assignee)

Comment 2

2 years ago
Created attachment 8747312 [details] [diff] [review]
Set a name for reply messages.

This is a hacky solution, but it works.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c270defeeb71
(Assignee)

Comment 3

2 years ago
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).
(Assignee)

Updated

2 years ago
Attachment #8747312 - Flags: review?(wmccloskey)
(Assignee)

Comment 5

2 years ago
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)
(Assignee)

Comment 6

2 years ago
Created attachment 8748001 [details] [diff] [review]
Use the name of the original message in Send for reply telemetry.

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)
(Assignee)

Updated

2 years ago
Attachment #8747312 - Attachment is obsolete: true
Attachment #8748001 - Flags: review?(wmccloskey) → review+

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/53d3b7cdb82f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Updated

2 years ago
Depends on: 1264820
See Also: bug 1264820
(Assignee)

Comment 9

2 years ago
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+

Updated

2 years ago
status-firefox47: --- → affected
status-firefox48: --- → affected

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb9344d9c9ad
status-firefox48: affected → fixed

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/3a218b10e323
status-firefox47: affected → fixed

Updated

2 years ago
tracking-e10s: --- → +
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.