Closed Bug 1748852 Opened 2 years ago Closed 2 years ago

Add a MOZ_ASSERT_UNLESS_FUZZING to IPC_FAIL caused error path(s)

Categories

(Core :: IPC, task, P3)

task

Tracking

()

RESOLVED FIXED
98 Branch
Tracking Status
firefox98 --- fixed

People

(Reporter: jstutte, Assigned: jstutte)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.

See Also: → 1747199
Attachment #9257844 - Attachment is obsolete: true
Assignee: nobody → jstutte
Severity: -- → S4
Priority: -- → P3
See Also: → 1748920

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.

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.

Flags: needinfo?(nika)

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

Flags: needinfo?(nika)
Attachment #9257848 - Attachment description: WIP: Bug 1748852: Assert on IPC_FAIL unless fuzzing. → Bug 1748852: Assert on IPC_FAIL unless fuzzing. r?#ipc-reviewers
See Also: → 1750497
See Also: → 1750525
Pushed by jstutte@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f8090459465
Assert on IPC_FAIL unless fuzzing. r=ipc-reviewers,nika,media-playback-reviewers,alwu

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

Flags: needinfo?(jstutte)

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

Flags: needinfo?(jstutte)
Blocks: 1751168
Pushed by jstutte@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
See Also: → 1752896
See Also: → 1253706
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: