1750525 - Give IPC_FAIL_NO_REASON a reason and check it well

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, task)

Description

[Jens Stutte [:jstutte]]

IPC_FAIL_NO_REASON should not be used and transformed into IPC_FAIL with reason.

In doing so, we should also verify if the reason really merits an IPC_FAIL given the possible fatal consequences for the calling actor.

Comment 1

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Recv__delete__ should IPC_FAIL only on impossible parameters. r?#dom-storage-reviewers

All HandleResponse variants never return false and some Recv__delete__ already ignored their return values. Let's harmonize this (should not change anything during runtime).

Comment 2

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Adjust BackgroundFactoryRequestChild::RecvPermissionChallenge IPC_FAIL cases. r?#dom-storage-reviewers

IPC_FAIL only when we see serious permission problems. Ignore dispatch failures as those already indicate dead actors.

Depends on D136131

Comment 3

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Adjust comment in BackgroundDatabaseChild::RecvPBackgroundIDBVersionChangeTransactionConstructor. r?#dom-storage-reviewers

Depends on D136132

Comment 4

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Do not IPC_FAIL in BackgroundRequestChild::RecvPreprocess on SendContinue dispatch failure. r?#dom-storage-reviewers

If SendContinue(rv) fails, our channel is already dead so there is no need for another IPC_FAIL.

Depends on D136133

Comment 5

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Do not IPC_FAIL in Factory::RecvDeleteMe on send failure. r?#dom-storage-reviewers

Depends on D136134

Comment 6

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Add reason to IPC_FAIL in Database::RecvBlocked if already closed. r?#dom-storage-reviewers

Depends on D136135

Comment 7

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason in Database::RecvClose and rely on IPC to crash when appropriate. r?#dom-storage-reviewers

Depends on D136136

Comment 8

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Do not IPC_FAIL in NormalTransaction::RecvDeleteMe on Send__delete__ failure. r?#dom-storage-reviewers

Depends on D136137

Comment 9

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Do not MOZ_CRASH_UNLESS_FUZZING for TransactionBase::RecvCommit failures. r?#dom-storage-reviewers

This allows us to not IPC_FAIL at all when fuzzing.

Depends on D136139

Comment 10

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Do not MOZ_CRASH_UNLESS_FUZZING for TransactionBase::RecvAbort failures. r?#dom-storage-reviewers

Depends on D136140

Comment 11

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Add reason to IPC_FAIL in NormalTransaction::RecvPBackgroundIDBRequestConstructor. r?#dom-storage-reviewers

Depends on D136141

Comment 12

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Add reason to IPC_FAIL in NormalTransaction::RecvPBackgroundIDBCursorConstructor. r?#dom-storage-reviewers

Depends on D136142

Comment 13

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Do not IPC_FAIL in VersionChangeTransaction::RecvDeleteMe on Send__delete__ failure. r?#dom-storage-reviewers

Depends on D136143

Comment 14

[Jens Stutte [:jstutte]]

Before I continue to apply the same patterns everywhere, a short feedback if I am heading to the right direction would be good.

Comment 15

[Jan Varga [:janv]]

Yeah, this needs to be discussed first. Can you provide a brief description of the new approach ?

Comment 16

[Jens Stutte [:jstutte]]

Well, I wanted to get concrete with some examples before discussing.

The basic patterns encountered so far are:

1. Do not IPC_FAIL on a failing dispatch, as this already indicates a dead channel/actor, just ignore to avoid actor delete ping-pong. Note that dropped messages will be reported already by the IPC sub-system, so no need to even warn.
2. Do not IPC_FAIL if we have a MOZ_CRASH_UNLESS_FUZZING in case we are fuzzing.
3. Add reasons to the rest.

The few remaining cases are real changes like D136136 in the sense of "if we can recover without harm, just ignore".

Comment 17

[Jan Varga [:janv]]

Well, I think we need to create a guide for this, maybe something already exists.
Like, when IPC_FAIL or MOZ_CRASH_UNLESS_FUZZING should be used what they actually do, what to do when we failed to send a message, etc.

Comment 18

[Jens Stutte [:jstutte]]

Some context is in bug 1748852 and bug 1748920. I assume, doing this will let us discover the patterns worth documenting best.

Comment 19

[Jens Stutte [:jstutte]]

I asked over there in bug 1748920 if pattern 1. is reasonable. The MOZ_CRASH_UNLESS_FUZZING case seems a bit specific to our code here and I assume that those have been added time ago for some specific diagnostics.

Comment 20

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Do not IPC_FAIL in Database::RecvDeleteMe on Send__delete__ failure. r?#dom-storage-reviewers

Depends on D136144

Comment 21

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvCreateObjectStore. r?#dom-storage-reviewers

Depends on D136161

Comment 22

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvDeleteObjectStore. r?#dom-storage-reviewers

Depends on D136580

Comment 23

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvRenameObjectStore. r?#dom-storage-reviewers

Depends on D136581

Comment 24

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvCreateIndex. r?#dom-storage-reviewers

Depends on D136582

Comment 25

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvDeleteIndex. r?#dom-storage-reviewers

Depends on D136583

Comment 26

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvRenameIndex. r?#dom-storage-reviewers

Depends on D136584

Comment 27

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason in VersionChangeTransaction::RecvPBackgroundIDBRequestConstructor. r?#dom-storage-reviewers

Depends on D136585

Comment 28

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason in VersionChangeTransaction::RecvPBackgroundIDBCursorConstructor. r?#dom-storage-reviewers

Depends on D136586

Comment 29

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason but do not fail on dispatch failure in Cursor<CursorType>::RecvDeleteMe. r?#dom-storage-reviewers

Depends on D136587

Comment 30

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in Cursor<CursorType>::RecvContinue. r?#dom-storage-reviewers

Depends on D136588

Comment 31

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in MutableFile::RecvGetFileId. r?#dom-storage-reviewers

Depends on D136589

Comment 32

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Do not IPC_FAIL on dispatch failure in Utils::RecvDeleteMe. r?#dom-storage-reviewers

Depends on D136590

Comment 33

[Jens Stutte [:jstutte]]

Attachment: Bug 1750525: Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in Utils::RecvGetFileReferences. r?#dom-storage-reviewers

Depends on D136591

Comment 34

[Jens Stutte [:jstutte]]

These should be all patches here. I followed in most of the cases the following rules:

• ignore dispatch failures when sending __delete__ messages
• remove MOZ_CRASH_UNLESS_FUZZING where we already return IPC_FAIL (given that this triggers already a MOZ_ASSERT_UNLESS_FUZZING)
• give the remaining IPC_FAIL a reason

In some cases there still might be more IPC_FAIL cases than strictly necessary, but if they do not bite us, that's probably ok and we do not need to spend too much time reasoning about them.

The remaining MOZ_CRASH_UNLESS_FUZZING should be handled in bug 1751371 as they are not directly IPC_FAIL related.

FWIW, here comes a try run on the entire patch stack.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1238ae983c59
Recv__delete__ should IPC_FAIL only on impossible parameters. r=dom-storage-reviewers,janv

Comment 36

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/1238ae983c59

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1422c1f67405
Adjust BackgroundFactoryRequestChild::RecvPermissionChallenge IPC_FAIL cases. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/1ebb8078cd98
Adjust comment in BackgroundDatabaseChild::RecvPBackgroundIDBVersionChangeTransactionConstructor. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/2614f6c4daa9
Do not IPC_FAIL in BackgroundRequestChild::RecvPreprocess on SendContinue dispatch failure. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/613811d0b6e4
Do not IPC_FAIL in Factory::RecvDeleteMe on send failure. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/8a28e73dc5b9
Add reason to IPC_FAIL in Database::RecvBlocked if already closed. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/61e2b1ced451
Give IPC_FAIL a reason in Database::RecvClose and rely on IPC to crash when appropriate. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/e6accb230bba
Do not IPC_FAIL in NormalTransaction::RecvDeleteMe on Send__delete__ failure. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/301c326b9c49
Do not MOZ_CRASH_UNLESS_FUZZING for TransactionBase::RecvCommit failures. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/95a706512282
Do not MOZ_CRASH_UNLESS_FUZZING for TransactionBase::RecvAbort failures. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/64a134d3e149
Add reason to IPC_FAIL in NormalTransaction::RecvPBackgroundIDBRequestConstructor. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/b6e7cf39f49c
Add reason to IPC_FAIL in NormalTransaction::RecvPBackgroundIDBCursorConstructor. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/fac86a1fa16b
Do not IPC_FAIL in VersionChangeTransaction::RecvDeleteMe on Send__delete__ failure. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/c1c687fa4812
Do not IPC_FAIL in Database::RecvDeleteMe on Send__delete__ failure. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/201cc80c4c92
Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvCreateObjectStore. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/b693a14d69d1
Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvDeleteObjectStore. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/6f5a3f68f847
Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvRenameObjectStore. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/161fdc257c31
Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvCreateIndex. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/fc7eaa6e55b4
Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvDeleteIndex. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/f9b134c67bdf
Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in VersionChangeTransaction::RecvRenameIndex. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/32ca87f4e55f
Give IPC_FAIL a reason in VersionChangeTransaction::RecvPBackgroundIDBRequestConstructor. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/4abc5fce18de
Give IPC_FAIL a reason in VersionChangeTransaction::RecvPBackgroundIDBCursorConstructor. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/5298cb5a77eb
Give IPC_FAIL a reason but do not fail on dispatch failure in Cursor<CursorType>::RecvDeleteMe. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/9aaa4aab943c
Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in Cursor<CursorType>::RecvContinue. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/c3175e8089b8
Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in MutableFile::RecvGetFileId. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/86207cdb9c4b
Do not IPC_FAIL on dispatch failure in Utils::RecvDeleteMe. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/abb14f60d11a
Give IPC_FAIL a reason and remove obsolete MOZ_CRASH_UNLESS_FUZZING in Utils::RecvGetFileReferences. r=dom-storage-reviewers,janv

Comment 38

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1422c1f67405
https://hg.mozilla.org/mozilla-central/rev/1ebb8078cd98
https://hg.mozilla.org/mozilla-central/rev/2614f6c4daa9
https://hg.mozilla.org/mozilla-central/rev/613811d0b6e4
https://hg.mozilla.org/mozilla-central/rev/8a28e73dc5b9
https://hg.mozilla.org/mozilla-central/rev/61e2b1ced451
https://hg.mozilla.org/mozilla-central/rev/e6accb230bba
https://hg.mozilla.org/mozilla-central/rev/301c326b9c49
https://hg.mozilla.org/mozilla-central/rev/95a706512282
https://hg.mozilla.org/mozilla-central/rev/64a134d3e149
https://hg.mozilla.org/mozilla-central/rev/b6e7cf39f49c
https://hg.mozilla.org/mozilla-central/rev/fac86a1fa16b
https://hg.mozilla.org/mozilla-central/rev/c1c687fa4812
https://hg.mozilla.org/mozilla-central/rev/201cc80c4c92
https://hg.mozilla.org/mozilla-central/rev/b693a14d69d1
https://hg.mozilla.org/mozilla-central/rev/6f5a3f68f847
https://hg.mozilla.org/mozilla-central/rev/161fdc257c31
https://hg.mozilla.org/mozilla-central/rev/fc7eaa6e55b4
https://hg.mozilla.org/mozilla-central/rev/f9b134c67bdf
https://hg.mozilla.org/mozilla-central/rev/32ca87f4e55f
https://hg.mozilla.org/mozilla-central/rev/4abc5fce18de
https://hg.mozilla.org/mozilla-central/rev/5298cb5a77eb
https://hg.mozilla.org/mozilla-central/rev/9aaa4aab943c
https://hg.mozilla.org/mozilla-central/rev/c3175e8089b8
https://hg.mozilla.org/mozilla-central/rev/86207cdb9c4b
https://hg.mozilla.org/mozilla-central/rev/abb14f60d11a

Comment 39

[Mathew Hodson]

It looks like all the patches landed. Can this be closed?

Comment 40

[Jens Stutte [:jstutte]]

Yes, thanks for reminding.