Avoid use of invalid ipc::FileDescriptor in memory report requests

RESOLVED FIXED in mozilla35

Status

()

Core
XPCOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

unspecified
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
PMemoryReportRequest (bug 946407) has been using the "invalid" case of ipc::FileDescriptor as a kind of in-band null value, sending an invalid DMD log file to indicate that DMD shouldn't be run.  Judging by bug 831307, this is wrong, and an IPDL union should be used instead.
(Assignee)

Comment 1

4 years ago
Created attachment 8486084 [details] [diff] [review]
bug1064529-dmd-maybe-hg0.diff

https://tbpl.mozilla.org/?tree=Try&rev=33af1e00e3f1 (and tested locally with a DMD build to verify that e10s-enabled DMD logging still works).
Attachment #8486084 - Flags: review?(bent.mozilla)
(Assignee)

Comment 2

4 years ago
Comment on attachment 8486084 [details] [diff] [review]
bug1064529-dmd-maybe-hg0.diff

Changing reviewer; the actual change here is to the memory reporter, and the question of ipc::FileDescriptor misuse is basically covered by noting the disappearance of the warning messages from the debug test logs.
Attachment #8486084 - Flags: review?(bent.mozilla) → review?(n.nethercote)
Comment on attachment 8486084 [details] [diff] [review]
bug1064529-dmd-maybe-hg0.diff

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

::: dom/ipc/ContentChild.cpp
@@ +219,5 @@
>  {
>      MOZ_COUNT_CTOR(MemoryReportRequestChild);
> +    if (aDMDFile.type() == MaybeFileDesc::TFileDescriptor) {
> +        mDMDFile = aDMDFile.get_FileDescriptor();
> +    }

I don't understand this bit. Where does TFileDescriptor come from -- is it generated? And is it safe to call type() on |aDMDFile| if it might be |void_t|? I feel like MaybeFileDesc needs a tag value that tells you which arm of the union is in use, but I might be missing something.

Also, it's not obvious what happens with |mDMDFile| when this test fails. Maybe it's default-constructed to some null-like value? Some kind of explicit initialization would make things clearer.
Comment on attachment 8486084 [details] [diff] [review]
bug1064529-dmd-maybe-hg0.diff

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

Ok, now that I understand IPDL unions this looks ok. Thanks.
Attachment #8486084 - Flags: review?(n.nethercote) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6579d448a271
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.