Closed Bug 1325655 Opened 3 years ago Closed 3 years ago

Crash in NS_QueryNotificationCallbacks<T>

Categories

(Core :: Networking, defect, critical)

Unspecified
Windows 8
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: marcia, Assigned: mayhemer)

References

Details

(Keywords: crash, Whiteboard: [necko-active])

Crash Data

Attachments

(2 files, 1 obsolete file)

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
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?
Assignee: nobody → honzab.moz
Whiteboard: [necko-active]
In 50, (100.0% in signature vs 18.68% overall) Addon "Adblock Plus" = true.
This is the #5 topcrash in Nightly 20170106030204.
Probably bug 1318759 shifted to yet another place.
Blocks: 1318759
clear and simple. no test coverage.
Attachment #8827481 - Flags: review?(jduell.mcbugs)
Attachment #8827481 - Flags: review?(jduell.mcbugs) → review+
no try since there is code path that would execute this in our test suite.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/703b1e442123
Let HttpChannelParent::ConnectChannel hard-fail when no nsHttpChannel is not found registered. r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/703b1e442123
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
(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.
(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.
Flags: needinfo?(honzab.moz)
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
Attachment #8827481 - Flags: approval-mozilla-beta?
Attachment #8827481 - Flags: approval-mozilla-aurora?
Comment on attachment 8827481 [details] [diff] [review]
v1 part 1/2 (fail when no channel found)

Fix a crash. Aurora53+.
Attachment #8827481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1334645
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.
Attachment #8827481 - Flags: approval-mozilla-beta?
The fix needs adjustments.  Killing the IPC channel probably makes the platform go crazy.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Attachment #8827481 - Attachment description: v1 (fail when no channel found) → v1 part 1/2 (fail when no channel found)
- 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.
Attachment #8836165 - Flags: review?(valentin.gosu)
refresh!
Attachment #8836165 - Attachment is obsolete: true
Attachment #8836165 - Flags: review?(valentin.gosu)
Attachment #8836166 - Flags: review?(valentin.gosu)
Attachment #8836166 - Flags: review?(valentin.gosu) → review+
Keywords: checkin-needed
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.
Attachment #8836166 - Flags: approval-mozilla-aurora?
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9685460cc7d0
Let HttpChannelParent::ConnectChannel delete itself if redirect target nsHttpChannel is not found registered. r=valentin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9685460cc7d0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
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+.
Attachment #8836166 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(honzab.moz)
(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 :)
Flags: needinfo?(honzab.moz)
Comment on attachment 8827481 [details] [diff] [review]
v1 part 1/2 (fail when no channel found)

Approval Request Comment: comment 12
Attachment #8827481 - Flags: approval-mozilla-beta?
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
Attachment #8836166 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1334645
Comment on attachment 8827481 [details] [diff] [review]
v1 part 1/2 (fail when no channel found)

crash fix for beta52
Attachment #8827481 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Attachment #8836166 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.