1264187 - crash in mozilla::dom::ProtoAndIfaceCache::~ProtoAndIfaceCache

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

froydnj, any ideas? You've touched this class in the not-too-distant past.

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Boris Zbarsky [: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.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: check for a ProtoAndIfaceCache before blindly destroying it

As discussed.

Comment 5

[Nathan Froyd [:froydnj]]

[Tracking Requested - why for this release]: Easy-to-fix crash, even if fixing it just pushes the crash to someplace else.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8741380 [details] [diff] [review]
check for a ProtoAndIfaceCache before blindly destroying it

r=me

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46033e80d7c2

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46033e80d7c2

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Only a few crashes for this in 45/46, good that we fixed it but too late for a last minute uplift to 46.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 11

[Nathan Froyd [:froydnj]]

Sure, that's beta at this point?  I can do that.

Comment 12

[Nathan Froyd [:froydnj]]

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

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8741380 [details] [diff] [review]
check for a ProtoAndIfaceCache before blindly destroying it

Not-null check, Beta47+

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fc51f034e4ac