Add a MOZ_ASSERT_UNLESS_FUZZING to IPC_FAIL caused error path(s)
Categories
(Core :: IPC, task, P3)
Tracking
()
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.
Assignee | ||
Comment 1•2 years ago
|
||
Updated•2 years ago
|
Assignee | ||
Comment 2•2 years ago
|
||
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
•
|
||
Adding this assert uncovers some interesting fallout and raises questions:
-
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.
-
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, asmCallback
is nulled out inClose
.
There might be more future intermittents, but those cases should probably be handled before landing.
Assignee | ||
Comment 4•2 years ago
|
||
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.
Assignee | ||
Comment 5•2 years ago
|
||
I made a proposal for those two occurrences in the patch, so let's move the discussion there, sorry for the noise.
Updated•2 years ago
|
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
Comment 7•2 years ago
•
|
||
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
Assignee | ||
Comment 8•2 years ago
|
||
So we triggered other shutdown cases in GMP, it seems.
Assignee | ||
Comment 9•2 years ago
|
||
Depends on D135238
Comment 10•2 years ago
|
||
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
Comment 11•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0a2734388072
https://hg.mozilla.org/mozilla-central/rev/4c6a24a3477e
Description
•