Closed Bug 1272160 Opened 4 years ago Closed 4 years ago

Assertion failure: js::GetObjectClass(global)->flags & (1<<7), at /home/khuey/dev/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/DOMJSClass.h:409 when getting a memory report

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 + fixed

People

(Reporter: khuey, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file gdb log
No description provided.
Flags: needinfo?(bzbarsky)
This blocks me from debugging bug 1272078.
Blocks: 1272078
Gah.  This is presumably a result of the fix for bug 1268845.  This line:

917     if (dom::HasProtoAndIfaceCache(mGlobalJSObject)) {

is assuming mGlobalJSObject is a JSCLASS_DOM_GLOBAL.  That happens to be the case with all xpconnect globals right now, I guess, but there are lots of codepaths that do NOT assume that, and the globals from bug 1268845 use XPCWrappedNativeScope but are NOT JSCLASS_DOM_GLOBAL.

Bobby, what's the story here?  Are we seriously expecting all mainthread JS globals to both have XPCWrappedNativeScopes _and_ be JSCLASS_DOM_GLOBAL with all the resulting baggage?  If not, seems like XPCWrappedNativeScope::AddSizeOfIncludingThis just needs to be checking for that flag before doing dom::HasProtoAndIfaceCache...
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
Oh, and once we decide what we want here, I guess we should uplift this too, along with bug 1268845.
Blocks: 1268845
No longer blocks: 1246153
(In reply to Boris Zbarsky [:bz] from comment #2)
> Gah.  This is presumably a result of the fix for bug 1268845.  This line:
> 
> 917     if (dom::HasProtoAndIfaceCache(mGlobalJSObject)) {
> 
> is assuming mGlobalJSObject is a JSCLASS_DOM_GLOBAL.  That happens to be the
> case with all xpconnect globals right now, I guess, but there are lots of
> codepaths that do NOT assume that, and the globals from bug 1268845 use
> XPCWrappedNativeScope but are NOT JSCLASS_DOM_GLOBAL.
> 
> Bobby, what's the story here?  Are we seriously expecting all mainthread JS
> globals to both have XPCWrappedNativeScopes _and_ be JSCLASS_DOM_GLOBAL with
> all the resulting baggage?

I don't think we should require that.

>  If not, seems like
> XPCWrappedNativeScope::AddSizeOfIncludingThis just needs to be checking for
> that flag before doing dom::HasProtoAndIfaceCache...

That seems reasonable to me offhand. Maybe we should have two functions: HasProtoAndIFaceCache, which checks the bit, and either returns false or delegates to DOMGlobalHasProtoAndIFaceCache, which asserts the bit. The former is the more obvious function to call, so that would reduce the chances for further screwups like this in the future.
Flags: needinfo?(bobbyholley)
[Tracking Requested - why for this release]:  Seems to be causing bug 1272746, which is a borderline topcrash.
I did audit the XPConnect callers of TraceProtoAndIfaceCache and DestroyProtoAndIfaceCache, and they all check JSCLASS_DOM_GLOBAL.
Attachment #8752406 - Flags: review?(khuey)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8752406 - Flags: review?(khuey) → review+
Comment on attachment 8752406 [details] [diff] [review]
Fix XPCWrappedNativeScope::AddSizeOfIncludingThis to not blindly poke at non-DOM globals as if they were DOM globals

Approval Request Comment
[Feature/regressing bug #]: Bug 1268845
[User impact if declined]: Crashes and other bad things to do with poking memory
   that isn't there.
[Describe test coverage new/current, TreeHerder]: Hard to trigger this: have to
   do a memory measurement while particular very temporary globals are live.
[Risks and why]: I think this is low-risk.  But hey, I said that about bug
   1268845 too!
[String/UUID change made/needed]: None.
Attachment #8752406 - Flags: review?(khuey)
Attachment #8752406 - Flags: review+
Attachment #8752406 - Flags: approval-mozilla-aurora?
Attachment #8752406 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/8da9fde2d10b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Tracking for 49.
Comment on attachment 8752406 [details] [diff] [review]
Fix XPCWrappedNativeScope::AddSizeOfIncludingThis to not blindly poke at non-DOM globals as if they were DOM globals

Fix an assertion, taking it in aurora
Attachment #8752406 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1272746
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.