Closed Bug 1349634 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Permission Manager, defect, critical)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + fixed

People

(Reporter: Ehsan, Assigned: Nika)

References

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(1 file, 2 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-d67b8fd2-3107-4e34-875a-eeaa72170322.
=============================================================
Duplicate of this bug: 1349749
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] → [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ]
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] → [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsurePermissi…
Crash Signature: mozilla::dom::ContentParent::EnsurePermissionsByKey ] → mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ xul.dll@0x25f9208 | PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey ]
[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.
Duplicate of this bug: 1349922
Crash Signature: mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ xul.dll@0x25f9208 | PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey ] → mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ xul.dll@0x25f9208 | PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::ChangeTable | PLDHashTable::Add | mozilla::dom::ContentParent::EnsurePermissionsBy…
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
Attachment #8850586 - Flags: review?(jduell.mcbugs)
Flags: needinfo?(michael)
oops - I forgot to make the nsresult DebugOnly<> when it lost its other uses.
Attachment #8850593 - Flags: review?(jduell.mcbugs)
Attachment #8850586 - Attachment is obsolete: true
Attachment #8850586 - Flags: review?(jduell.mcbugs)
Crash Signature: mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ xul.dll@0x25f9208 | PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::ChangeTable | PLDHashTable::Add | mozilla::dom::ContentParent::EnsurePermissionsBy… → mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::ChangeTable | PLDHashTable::Add | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ xul.dll@0x25f9208 | PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsB…
Tracking 55+ for this nightly crash regression.
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?
Attachment #8850593 - Flags: review?(jduell.mcbugs) → review?(wmccloskey)
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.
Attachment #8850593 - Flags: review?(wmccloskey) → review-
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?
Crash Signature: mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ mozilla::net::HttpChannelParent::OnStartRequest] → mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ mozilla::net::HttpChannelParent::OnStartRequest] [@ mozilla::dom::ContentParent::EnsurePermissionsByKey]
ni? wrt to billm's comment 9.
Flags: needinfo?(jduell.mcbugs)
MozReview-Commit-ID: EXHeRpN36yU
Attachment #8851080 - Flags: review?(wmccloskey)
Attachment #8850593 - Attachment is obsolete: true
Duplicate of this bug: 1350252
Crash Signature: mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ mozilla::net::HttpChannelParent::OnStartRequest] [@ mozilla::dom::ContentParent::EnsurePermissionsByKey] → mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ mozilla::net::HttpChannelParent::OnStartRequest] [@ mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ 0x0 | mozilla::dom::ContentParent::TransmitPermissionsFor] [@ mozilla::dom::ContentPa…
Attachment #8851080 - Flags: review?(wmccloskey) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/314479dc2403
Ensure that HttpChannelParent isn't closed before accessing its Manager(), r=billm
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).
Crash Signature: mozilla::dom::ContentParent::TransmitPermissionsFor] → mozilla::dom::ContentParent::TransmitPermissionsFor] [@ nsTHashtable<T>::GetEntry | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ]
Duplicate of this bug: 1350571
https://hg.mozilla.org/mozilla-central/rev/314479dc2403
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
No sign of EnsurePermissionsByKey crashes in 20170326030204, after being the #2 windows topcrash in Nightly 20170325030203. Good.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1349382
See Also: → 1424490
Flags: needinfo?(jduell.mcbugs)
You need to log in before you can comment on or make changes to this bug.