1748852 - Add a MOZ_ASSERT_UNLESS_FUZZING to IPC_FAIL caused error path(s)

Status: RESOLVED FIXED

(Core :: IPC, task, P3)

Description

[Jens Stutte [:jstutte]]

In the investigation in bug 1747199 it was helpful to have a MOZ_ASSERT_UNLESS_FUZZING inside the IPCResult::Fail implementation to detect unexpected failures.

Indeed we would never expect to see IPC_FAIL in a controlled environment on our CI (unless we are fuzzing, of course).

As per discussion with Nika on matrix, a natural place for a MOZ_ASSERT_UNLESS_FUZZING would be in ProcessingError for nonMsgDropped errors here, but this can be overridden. While it feels a bit weird to only assert in IPCResult::Fail and not in other codepaths which can reach ProcessingError, this is perhaps fine to catch an excessive usage of IPC_FAIL(_NO_REASON) return values.

Comment 1

[Jens Stutte [:jstutte]]

Attachment: WIP: Bug 1748852: Assert on IPC_FAIL failure unless fuzzing.

Comment 2

[Jens Stutte [:jstutte]]

Attachment: Bug 1748852: Assert on IPC_FAIL unless fuzzing. r?#ipc-reviewers

Comment 3

[Jens Stutte [:jstutte]]

Adding this assert uncovers some interesting fallout and raises questions:

1. Should we use IPC_FAIl to signal a shutdown ? My guess would be no, as shutdown is not an unexpected state to be in and potentially killing a child actor during shutdown could result in hangs elsewhere.

2. In GMPVideoDecoderParent::RecvDecoded inlining obfuscates which of the two is triggering. For the second reading this comment suggests to ignore the frame, which would mean to do nothing and return IPC_OK. The first seems to be again a shutdown case, as mCallback is nulled out in Close.

There might be more future intermittents, but those cases should probably be handled before landing.

Comment 4

[Jens Stutte [:jstutte]]

Hi Nika, the shutdown question above makes me think if we should have a special IPC_FAIL_SHUTDOWN value that does not kill the actor but signals the shutdown state somehow? I ignore the implications, but it sounds like this could be a quite complicated change. The alternative would be to just return IPC_OK and do nothing, but that might have unexpected consequences in the calling actor, too. We also could just say that those specific functions need a success input/output value such that the caller can decide what to do on such recoverable errors.

Comment 5

[Jens Stutte [:jstutte]]

I made a proposal for those two occurrences in the patch, so let's move the discussion there, sorry for the noise.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f8090459465
Assert on IPC_FAIL unless fuzzing. r=ipc-reviewers,nika,media-playback-reviewers,alwu

Comment 7

[Norisz Fay [:noriszfay]]

Backed out for causing assertion failures on ProtocolUtils.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/39f3ce7121a8e9a0568ba71b3e0eec9f41aa9aab

Push with failures: failure

Failure log(s): failure log

Failure line(s): Assertion failure: false (Please ensure to IPC_FAIL only when in an unrecoverable, unexpected state.), at /builds/worker/checkouts/gecko/ipc/glue/ProtocolUtils.cpp:68

Comment 8

[Jens Stutte [:jstutte]]

So we triggered other shutdown cases in GMP, it seems.

Comment 9

[Jens Stutte [:jstutte]]

Attachment: Bug 1748852: Do not IPC_FAIL on GMP shutdown and give remaining fails a reason. r?#media-playback-reviewers

Depends on D135238

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a2734388072
Assert on IPC_FAIL unless fuzzing. r=ipc-reviewers,nika,media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/4c6a24a3477e
Do not IPC_FAIL on GMP shutdown and give remaining fails a reason. r=media-playback-reviewers,bryce

Comment 11

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0a2734388072
https://hg.mozilla.org/mozilla-central/rev/4c6a24a3477e