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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: 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).

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 3

[Nicholas Nethercote [inactive]]

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 4

[Nicholas Nethercote [inactive]]

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.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1064529-dmd-maybe-hg1.diff

Rebase; carrying over r+.

https://tbpl.mozilla.org/?tree=Try&rev=15730d9d093f
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=15730d9d093f

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6579d448a271

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6579d448a271