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

VERIFIED FIXED in Firefox 55

Status

()

Core
Permission Manager
--
critical
VERIFIED FIXED
a year ago
7 months ago

People

(Reporter: Ehsan, Assigned: Nika, NeedInfo)

Tracking

({crash, regression, topcrash})

unspecified
mozilla55
Unspecified
Windows 10
crash, regression, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55+ fixed)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

a year ago
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 ]
status-firefox55: --- → affected
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::EnsureP&hellip;
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip; → [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip;
[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.
status-firefox54: --- → unaffected
tracking-firefox55: --- → ?
Keywords: regression, topcrash
(Assignee)

Updated

a year ago
Duplicate of this bug: 1349922
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip; → [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip;
(Assignee)

Comment 4

a year ago
Created attachment 8850586 [details] [diff] [review]
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
Attachment #8850586 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

a year ago
Flags: needinfo?(michael)
(Assignee)

Comment 5

a year ago
Created attachment 8850593 [details] [diff] [review]
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.
Attachment #8850593 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

a year ago
Attachment #8850586 - Attachment is obsolete: true
Attachment #8850586 - Flags: review?(jduell.mcbugs)
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip; → [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip;
Tracking 55+ for this nightly crash regression.
tracking-firefox55: ? → +
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: [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip; → [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip;
(Assignee)

Comment 10

a year ago
ni? wrt to billm's comment 9.
Flags: needinfo?(jduell.mcbugs)
(Assignee)

Comment 11

a year ago
Created attachment 8851080 [details] [diff] [review]
Ensure that HttpChannelParent isn't closed before accessing its Manager()

MozReview-Commit-ID: EXHeRpN36yU
Attachment #8851080 - Flags: review?(wmccloskey)
(Assignee)

Updated

a year ago
Attachment #8850593 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Duplicate of this bug: 1350252
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip; → [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip;
Attachment #8851080 - Flags: review?(wmccloskey) → review+

Comment 13

a year ago
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
Blocks: 1350313
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: [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip; → [@ PLDHashTable::Search | mozilla::dom::ContentParent::EnsurePermissionsByKey] [@ PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey ] [@ PLDHashTable::Add | mozilla::dom::ContentParent::EnsureP&hellip;

Updated

a year ago
Duplicate of this bug: 1350571

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/314479dc2403
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
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
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox-esr52: --- → unaffected
Duplicate of this bug: 1349382

Updated

7 months ago
See Also: → bug 1424490
You need to log in before you can comment on or make changes to this bug.