1325655 - Crash in NS_QueryNotificationCallbacks<T>

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-ca636fe8-9324-4579-972f-7b3112161223.
=============================================================

Seen while looking at Nightly crash stats: http://bit.ly/2hQqu7F. Crashes started with 20161221030226 build.

Possible regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=567894f026558e6dada617a3998f29aed06ac7d8&tochange=c36fbe84042debef0a5d58b7fc88185b401762ce

Comment 1

[Michal Novotny [:michal]]

It looks like channel is null, which would mean that RedirectChannelRegistrar::LinkChannels didn't find the real channel for that id. Honza, can you have a look at it?

Comment 2

[Marco Castelluccio [:marco]]

In 50, (100.0% in signature vs 18.68% overall) Addon "Adblock Plus" = true.

Comment 3

[Nicholas Nethercote [inactive]]

This is the #5 topcrash in Nightly 20170106030204.

Comment 4

[Honza Bambas (:mayhemer)]

Probably bug 1318759 shifted to yet another place.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1 part 1/2 (fail when no channel found)

clear and simple. no test coverage.

Comment 6

[Honza Bambas (:mayhemer)]

no try since there is code path that would execute this in our test suite.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/703b1e442123
Let HttpChannelParent::ConnectChannel hard-fail when no nsHttpChannel is not found registered. r=jduell

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/703b1e442123

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Please request Aurora/Beta approval on this when you get a chance.

Not until bug 1334645 comes to some conclusion.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> > Please request Aurora/Beta approval on this when you get a chance.
> 
> Not until bug 1334645 comes to some conclusion.

No time to investigate that bug and probably the relation to this bug is a red herring.  Asking for a now.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8827481 [details] [diff] [review]
v1 part 1/2 (fail when no channel found)

Applies cleanly on m-a, m-b


Approval Request Comment
[Feature/Bug causing the regression]: e10s, async redirects (landed years ago)
[User impact if declined]: parent process crash
[Is this code covered by automated tests?]: yes, but this case is not!
[Has the fix been verified in Nightly?]: yes, according crash stats
[Needs manual test from QE? If yes, steps to reproduce]: i unfortunately don't know about a way to repro this :(
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: depends on how it turns out with bug 1334645 which is tho inconclusive if caused by landing this on nightly (crash stats sucks)
[Why is the change risky/not risky?]: this is a second or a third fix in this area.  the crash (and all the crashes fixed before this bug) are caused by an addon or combination of addons and a so far unknown scenario/configuration.  it may be possible this patch will just 'shift' the problem further down - that another crash or something may appear further in the logic paths.
[String changes made/needed]: none

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8827481 [details] [diff] [review]
v1 part 1/2 (fail when no channel found)

Fix a crash. Aurora53+.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/73aac08829e8

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8827481 [details] [diff] [review]
v1 part 1/2 (fail when no channel found)

Dropping a?beta since this is probably cause another, harder to fix kind of crash, now appearing (probably freshly - yet unconfirmed) on aurora after this has been landed there.

Comment 16

[Honza Bambas (:mayhemer)]

The fix needs adjustments.  Killing the IPC channel probably makes the platform go crazy.

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: v1 part 2/2 (don't kill the IPC with return false, but regularly delete self)

- return false from ipc Recv really is something to never use!
- I moved the block before the line we rewrite mChannel since I want to leave the current mChannel non null ; there are not many non-nullness checks around this code :/
- please check that Delete() is the right thing to do and that when the child sends some IPC in the meantime we don't crash again

Valentin, I remember you were digging here recently, hence you may be the best reviewer for this.

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v1 part 2/2 (don't kill the IPC with return false, but regularly delete self)

refresh!

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8836166 [details] [diff] [review]
v1 part 2/2 (don't kill the IPC with return false, but regularly delete self)

Approval Request Comment - see comment 12.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9685460cc7d0
Let HttpChannelParent::ConnectChannel delete itself if redirect target nsHttpChannel is not found registered. r=valentin

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/9685460cc7d0

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8836166 [details] [diff] [review]
v1 part 2/2 (don't kill the IPC with return false, but regularly delete self)

Fix a crash. Aurora53+.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/434d0089e1ad

Comment 24

[Ryan VanderMeulen [:RyanVM]]

This is looking really good on crash-stats since the second patch landed on Trunk and Aurora. How are we feeling about requesting uplift to Beta?

Comment 25

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> This is looking really good on crash-stats since the second patch landed on
> Trunk and Aurora. How are we feeling about requesting uplift to Beta?

Positively :)

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 8827481 [details] [diff] [review]
v1 part 1/2 (fail when no channel found)

Approval Request Comment: comment 12

Comment 27

[Honza Bambas (:mayhemer)]

Comment on attachment 8836166 [details] [diff] [review]
v1 part 2/2 (don't kill the IPC with return false, but regularly delete self)

Approval Request Comment: comment 12

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 8827481 [details] [diff] [review]
v1 part 1/2 (fail when no channel found)

crash fix for beta52

Comment 30

[Julien Cristau [:jcristau]]

Comment on attachment 8836166 [details] [diff] [review]
v1 part 2/2 (don't kill the IPC with return false, but regularly delete self)

crash fix for beta52

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d4c847e22d29
https://hg.mozilla.org/releases/mozilla-beta/rev/856362367643

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d4c847e22d29
https://hg.mozilla.org/releases/mozilla-esr52/rev/856362367643