1272160 - 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

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: gdb log

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This blocks me from debugging bug 1272078.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and once we decide what we want here, I guess we should uplift this too, along with bug 1268845.

Comment 4

[Bobby Holley (:bholley)]

(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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

[Tracking Requested - why for this release]:  Seems to be causing bug 1272746, which is a borderline topcrash.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix XPCWrappedNativeScope::AddSizeOfIncludingThis to not blindly poke at non-DOM globals as if they were DOM globals

I did audit the XPConnect callers of TraceProtoAndIfaceCache and DestroyProtoAndIfaceCache, and they all check JSCLASS_DOM_GLOBAL.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8da9fde2d10b

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8da9fde2d10b

Comment 10

[Marcia Knous [:marcia]]

Tracking for 49.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bcac6066d926