1349634 - Crash in PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey

Status: VERIFIED FIXED

(Core :: Permission Manager, defect)

Description

[(no longer active)]

This bug was filed from the Socorro interface and is 
report bp-d67b8fd2-3107-4e34-875a-eeaa72170322.
=============================================================

Comment 2

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: Nightly top crash regression.

#2 crash on the 3-23 Windows Nightly.

#3 crash on the 3-23 OSX and Linux Nightly.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Ensure that the HttpChannelParent has a PContentParent before calling TransmitPermissionsFor on it

This is my current best guess as to what this crash is caused by. The crash
appears from what I can tell to be occuring because the methods to
TransmitPermissionsFor are being called on an invalid ContentParent pointer,
which is causing the hashtable lookup to fail (as there is no HashTable).

This changes the code to actually null-check the PNeckoParent and PContentParent
objects before calling methods on them.

MozReview-Commit-ID: EXHeRpN36yU

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Ensure that the HttpChannelParent has a PContentParent before calling TransmitPermissionsFor on it

oops - I forgot to make the nsresult DebugOnly<> when it lost its other uses.

Comment 6

[Marcia Knous [:marcia]]

Tracking 55+ for this nightly crash regression.

Comment 7

[Jason Duell]

Comment on attachment 8850593 [details] [diff] [review]
Ensure that the HttpChannelParent has a PContentParent before calling TransmitPermissionsFor on it

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

Looks OK to me, except I'm wondering if we're in Deep Trouble if we even get here and should cancel the channel or anything like that (it's bad to not have PNecko and/or it's Manager()?).  Bill, does this look ok to you?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8850593 [details] [diff] [review]
Ensure that the HttpChannelParent has a PContentParent before calling TransmitPermissionsFor on it

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

I suspect you just want to check !mIPCClosed here. If mIPCClosed is true, then calling Manager() is illegal anyway.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I do wonder if we should just check mIPCClosed at the top of the function and exit in that case. We correctly check it before doing any other IPC-related stuff, but it's kind of a footgun. Jason would know better though. Maybe we need to clean up some stuff?

Comment 10

[Nika Layzell [:nika] (ni? for response)]

ni? wrt to billm's comment 9.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Ensure that HttpChannelParent isn't closed before accessing its Manager()

MozReview-Commit-ID: EXHeRpN36yU

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/314479dc2403
Ensure that HttpChannelParent isn't closed before accessing its Manager(), r=billm

Comment 14

[Calixte Denizet (:calixte)]

There are 60 crashes with signature "nsTHashtable<T>::GetEntry | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey" on nightly 55 with buildid 20170324030205 (no crash before).

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/314479dc2403

Comment 17

[Nicholas Nethercote [inactive]]

No sign of EnsurePermissionsByKey crashes in 20170326030204, after being the #2 windows topcrash in Nightly 20170325030203. Good.