1436156 - CHECK() in Chromium IPC code should be fatal when not fuzzing

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

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

Bug 777067 changed the CHECK() macro in Chromium-derived IPC code, and by extension other variants like DCHECK(), from a fatal assertion to a warning.  This is not conditional on fuzzing; it applies to all builds, all the time.

We really shouldn't be skipping assertions like that outside of fuzz testing; I'd been assuming that I could depend on assertions like [1] to make problems immediately obvious, but now they log one message (easily missed in all the usual stderr junk) and proceed to do bad things.

Fortunately, there don't appear to be any security-sensitive uses of CHECK() at present, but it really doesn't seem like a good idea to have something that looks like the equivalent of MOZ_RELEASE_ASSERT but is basically NS_WARN_IF.


[1] https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/ipc/chromium/src/base/file_descriptor_shuffle.cc#28

Comment 1

[Christoph Diehl [:posidron]]

Attachment: faulty.check.diff

Comment 2

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

Comment on attachment 8956289 [details] [diff] [review]
faulty.check.diff

Review of attachment 8956289 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aef35642e5f
CHECK() in Chromium IPC code should be fatal when not fuzzing. r=jld

Comment 4

[Natalia Csoregi [:nataliaCs]]

Backed out for

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/406e69f3fd2e8ac5c2254556a72332a02222723b

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7aef35642e5facf784d9a41b96e54cf35f6fb5a3

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166178145&repo=mozilla-inbound&lineNumber=9716

[task 2018-03-06T11:29:51.389Z] 11:29:51     INFO - TEST-START | dom/indexedDB/test/browser_permissionsPromptAllow.js
[task 2018-03-06T11:29:51.434Z] 11:29:51     INFO - GECKO(3378) | ++DOCSHELL 0xe37a8800 == 3 [pid = 3495] [id = {8b344ef0-1ddc-44a8-bcde-a9274a4eb8b9}]
[task 2018-03-06T11:29:51.435Z] 11:29:51     INFO - GECKO(3378) | ++DOMWINDOW == 7 (0xf714b480) [pid = 3495] [serial = 7] [outer = (nil)]
[task 2018-03-06T11:29:51.538Z] 11:29:51     INFO - GECKO(3378) | ++DOMWINDOW == 8 (0xe37aa400) [pid = 3495] [serial = 8] [outer = 0xf714b480]
[task 2018-03-06T11:29:52.024Z] 11:29:52     INFO - GECKO(3378) | ++DOMWINDOW == 9 (0xe37a9800) [pid = 3495] [serial = 9] [outer = 0xf714b480]
[task 2018-03-06T11:29:52.329Z] 11:29:52     INFO - GECKO(3378) | [Child 3495, Main Thread] ###!!! ABORT: Inserting duplicate item: file /builds/worker/workspace/build/src/ipc/chromium/src/base/id_map.h, line 60
[task 2018-03-06T11:29:52.329Z] 11:29:52     INFO - GECKO(3378) | #01: mozilla::Logger::~Logger [ipc/chromium/src/base/logging.cc:48]
[task 2018-03-06T11:29:52.329Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.331Z] 11:29:52     INFO - GECKO(3378) | #02: IDMap<nsCOMPtr<nsIEventTarget> >::AddWithID [ipc/chromium/src/base/logging.h:62]
[task 2018-03-06T11:29:52.332Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.333Z] 11:29:52     INFO - GECKO(3378) | #03: mozilla::ipc::IToplevelProtocol::SetEventTargetForActorInternal [ipc/glue/ProtocolUtils.cpp:888]
[task 2018-03-06T11:29:52.334Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.336Z] 11:29:52     INFO - GECKO(3378) | #04: mozilla::dom::indexedDB::BackgroundFactoryRequestChild::RecvPermissionChallenge [dom/indexedDB/ActorsChild.cpp:2001]
[task 2018-03-06T11:29:52.338Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.339Z] 11:29:52     INFO - GECKO(3378) | #05: mozilla::dom::indexedDB::PBackgroundIDBFactoryRequestChild::OnMessageReceived [s3:gecko-generated-sources:cd14fd58c8fba1730941f9292daee57b67028ebc266682bb0f9dc82830a0263979b766bf0f31a72bef0087ae521bf018d18cae3d6d98a3ddcbca39a2560d6451/ipc/ipdl/PBackgroundIDBFactoryRequestChild.cpp::145]
[task 2018-03-06T11:29:52.341Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.342Z] 11:29:52     INFO - GECKO(3378) | #06: mozilla::ipc::MessageChannel::DispatchAsyncMessage [ipc/glue/MessageChannel.h:664]
[task 2018-03-06T11:29:52.343Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.344Z] 11:29:52     INFO - GECKO(3378) | #07: mozilla::ipc::MessageChannel::DispatchMessage [ipc/glue/MessageChannel.cpp:2065]
[task 2018-03-06T11:29:52.345Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.347Z] 11:29:52     INFO - GECKO(3378) | #08: mozilla::ipc::MessageChannel::RunMessage [ipc/glue/MessageChannel.cpp:1909]
[task 2018-03-06T11:29:52.354Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.356Z] 11:29:52     INFO - GECKO(3378) | #09: mozilla::ipc::MessageChannel::MessageTask::Run [ipc/glue/MessageChannel.cpp:1943]
[task 2018-03-06T11:29:52.357Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.359Z] 11:29:52     INFO - GECKO(3378) | #10: mozilla::SchedulerGroup::Runnable::Run [xpcom/threads/SchedulerGroup.cpp:413]
[task 2018-03-06T11:29:52.360Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.364Z] 11:29:52     INFO - GECKO(3378) | #11: nsThread::ProcessNextEvent [mfbt/Maybe.h:100]

Comment 5

[Natalia Csoregi [:nataliaCs]]

Backed out for failing ipc/chromium/src/base/id_map.h:60 ^

Comment 6

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

That looks related to bug 1317844.  I'll investigate.

Comment 7

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

I'm just going to put in a workaround for bug 1445121 to get this landed.

Comment 9

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

Attachment: Bug 1436156 - CHECK() in Chromium IPC code should be fatal when not fuzzing.

Includes a workaround for bug 1445121.

Review commit: https://reviewboard.mozilla.org/r/235886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235886/

Comment 10

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

(If there's another IPC reviewer who knows more about event targets, feel free to redirect the request.)

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8967215 [details]
Bug 1436156 - CHECK() in Chromium IPC code should be fatal when not fuzzing.

https://reviewboard.mozilla.org/r/235886/#review241788

I can't decide whether it says good things about our IPC code that we don't have to fix up more things after tweaking this.

::: ipc/chromium/src/base/logging.h:122
(Diff revision 1)
> +#ifdef FUZZING
>  #define CHECK(condition) LOG_IF(WARNING, condition)

Asking because I don't know: do the fuzzing people comb through the logs afterward to find messages like this and see if they are sensical?  Or is the desire to avoid the random fuzz data crashing the browser in weird places, and botching up the fuzzing runs?

Comment 12

[Christoph Diehl [:posidron]]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> Asking because I don't know: do the fuzzing people comb through the logs
> afterward to find messages like this and see if they are sensical?  Or is
> the desire to avoid the random fuzz data crashing the browser in weird
> places, and botching up the fuzzing runs?

The latter.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b413e24051f
CHECK() in Chromium IPC code should be fatal when not fuzzing. r=froydnj

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8b413e24051f