1351558 - Crash in mozalloc_abort | NS_DebugBreak | mozilla::ipc::LogicError | mozilla::net::PHttpChannel::Transition

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-19ce3974-195f-4c6a-aae6-ee3782170328.
=============================================================

There are 7 crashes in nightly 55 with buildid 20170328095415. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1320744.

[1] https://hg.mozilla.org/mozilla-central/rev?node=de9bccfe404e04fce4ba2ace7561e01404459448

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Checking and keep the ni?

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

This crash is about trying to send IPC messages on a dead IPC actor. I suspect that we trigger the `TrySendDeletingChannel` more than once. There are two boolean flags we depends on that in not thread safe: `mIPCOpen` and `mKeptAlive`. [1]

My first guess is that making `mIPCOpen` atomic might be sufficient. Trying to reproduce it locally to prove my theory.

[1] https://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/netwerk/protocol/http/HttpChannelChild.h#233-234

Comment 3

[Calixte Denizet (:calixte)]

An other signature with 4 crashes where the backtrace has some similarities with the previous one:
https://crash-stats.mozilla.com/report/index/cd395b8b-3d36-4843-9e01-c85cb2170328

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1351558 - double check mIPCOpen flag before we send IPC message. .

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

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1351558 - make sure PHttpChannel destroy procedure will only be done once.

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

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@mayhemer maybe too busy to review my patch today. Ask @dragana to do this quick review since she is familiar with the code as well.

@dragana, HttpChannelChild can now destroys itself on non-main-thread. From the code inspection, `mIPCOpen` is not thread safe so we might call TrySendDeletingChannel more than once.

I implement two safe mechanisms here:
  1) make `mIPCOpen` atomic and check again before SendDeletingChannel is called;
  2) introduce `mDeletingChannelSent` to ensure SendDeletingChannel procedure is run at most once within the HttpChannelChild life cycle.

Comment 7

[Dragana Damjanovic [:dragana]]

Comment on attachment 8852389 [details]
Bug 1351558 - double check mIPCOpen flag before we send IPC message. .

https://reviewboard.mozilla.org/r/124632/#review127240

Comment 8

[Dragana Damjanovic [:dragana]]

Comment on attachment 8852390 [details]
Bug 1351558 - make sure PHttpChannel destroy procedure will only be done once.

https://reviewboard.mozilla.org/r/124634/#review127250

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/065baa8ca93c
double check mIPCOpen flag before we send IPC message. r=dragana.
https://hg.mozilla.org/integration/autoland/rev/a1d6a86da1e3
make sure PHttpChannel destroy procedure will only be done once. r=dragana

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/065baa8ca93c
https://hg.mozilla.org/mozilla-central/rev/a1d6a86da1e3