Closed Bug 1264187 Opened 4 years ago Closed 4 years ago

crash in mozilla::dom::ProtoAndIfaceCache::~ProtoAndIfaceCache

Categories

(Core :: DOM: Core & HTML, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 - wontfix
firefox47 + fixed
firefox48 + fixed

People

(Reporter: njn, Assigned: froydnj)

Details

(Keywords: crash, Whiteboard: btpp-active)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-4133abd0-5f1d-4144-9a18-6a69a2160412.
=============================================================

Looks like a null deref where |this| is null. It has occurred 21 times in the past week, across a number of Firefox versions.
froydnj, any ideas? You've touched this class in the not-too-distant past.
Flags: needinfo?(nfroyd)
We're calling the destructor, but we have a null cache (|this|).  The only place we destroy cache objects is DestroyProtoAndIfaceCache:

http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.h#530

so we must be getting to a call of DestroyProtoAndIfaceCache without ever calling AllocateProtoAndIfaceCache on the global:

http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.h#478

I guess the straightforward solution here is to check HasProtoAndIfaceCache before calling the destructor.  It seems a little odd that we could run into this sort of case, since AllocateProtoAndIfaceCache is called immediate after creating the global object.  But I see TraceProtoAndIfaceCache has a similar check, and there's a comment in HasProtoAndIfaceCache that we can GC while creating the global, so maybe that's the case we're running into here.

bz, does that sound reasonable?  If so, I'll write up a patch.
Flags: needinfo?(nfroyd) → needinfo?(bzbarsky)
I guess it could happen that we try to allocate a global, create the global object itself, then go to init some of the standard classes or whatnot on it and OOM.  That would make JS_NewGlobalObject return null, so we would never AllocateProtoAndIfaceCache, but then when we finalize the object we created and never got our hands on we could end up in the situation you describe.

So yeah, adding a check here makes sense.
Flags: needinfo?(bzbarsky)
Whiteboard: btpp-active
Assignee: nobody → nfroyd
OS: Mac OS X → All
Hardware: Unspecified → All
[Tracking Requested - why for this release]: Easy-to-fix crash, even if fixing it just pushes the crash to someplace else.
Comment on attachment 8741380 [details] [diff] [review]
check for a ProtoAndIfaceCache before blindly destroying it

r=me
Attachment #8741380 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/46033e80d7c2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Only a few crashes for this in 45/46, good that we fixed it but too late for a last minute uplift to 46.
Want to uplift to 47 ? Possibly worth doing.  I am going through tracking requests for 48 right now and noticed this is still affected for 47. There is 1 crash in the last week with this signature in 47.01b.
Flags: needinfo?(nfroyd)
Sure, that's beta at this point?  I can do that.
Flags: needinfo?(nfroyd)
Comment on attachment 8741380 [details] [diff] [review]
check for a ProtoAndIfaceCache before blindly destroying it

Approval Request Comment
[Feature/regressing bug #]: new dom bindings
[User impact if declined]: small amount of crashes will continue to take place
[Describe test coverage new/current, TreeHerder]: this is extremely well-covered code
[Risks and why]: low risk
[String/UUID change made/needed]: none
Attachment #8741380 - Flags: approval-mozilla-beta?
Comment on attachment 8741380 [details] [diff] [review]
check for a ProtoAndIfaceCache before blindly destroying it

Not-null check, Beta47+
Attachment #8741380 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.