Closed
Bug 1264187
Opened 8 years ago
Closed 8 years ago
crash in mozilla::dom::ProtoAndIfaceCache::~ProtoAndIfaceCache
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: n.nethercote, Assigned: froydnj)
Details
(Keywords: crash, Whiteboard: btpp-active)
Crash Data
Attachments
(1 file)
1.49 KB,
patch
|
bzbarsky
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
froydnj, any ideas? You've touched this class in the not-too-distant past.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Comment 3•8 years ago
|
||
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)
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → nfroyd
OS: Mac OS X → All
Hardware: Unspecified → All
Assignee | ||
Comment 5•8 years ago
|
||
[Tracking Requested - why for this release]: Easy-to-fix crash, even if fixing it just pushes the crash to someplace else.
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
status-firefox47:
--- → affected
tracking-firefox46:
--- → ?
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → ?
Comment 6•8 years ago
|
||
Comment on attachment 8741380 [details] [diff] [review] check for a ProtoAndIfaceCache before blindly destroying it r=me
Attachment #8741380 -
Flags: review?(bzbarsky) → review+
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/46033e80d7c2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 9•8 years ago
|
||
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•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
Sure, that's beta at this point? I can do that.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 12•8 years ago
|
||
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+
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•