615834 - warning: inline function ‘JSCompartment* JSObject::getCompartment() const’ used but never defined

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[timeless]

nsWebSocket.cpp
../../../../dist/include/jsobj.h:1130: warning: inline function ‘JSCompartment* JSObject::getCompartment() const’ used but never defined

This comes up a bit, it's rather annoying. I believe the wrong fix is a patch to an xpconnect header. Alternatives include having jsobj.h be aware of whether it's being included by spidermonkey code or external code and automatically including jsgc.h, or just having jsobj.h automatically include jsgc.h.

from my perspective as an embedder, the fact that I have to include <jsgc.h> to use declarations from <jsobj.h> makes no sense and feels like an implementation detail that I shouldn't have to know about.

Comment 1

[timeless]

Attachment: i do not like this green eggs and ham

Comment 2

[timeless]

jsobj.h declares 'getCompartment' as inline
jsgc.h defines getCompartment
but jsobj.h doesn't include jsgc.h

so if you (xpconnect) include jsobj.h
and don't include jsgc.h
then when you try to use getCompartment which is declared inline
the compiler *can't* inline it, because it hasn't met it -- which is the compiler's complaint.

it's perfectly reasonable. you asked it to do something w/o giving it what it needs to do it.

http://mxr.mozilla.org/mozilla-central/ident?i=getCompartment&tree=mozilla-central&filter=&strict=1
js/src/jsobj.h
line 1130 -- inline JSCompartment *getCompartment() const;

there you asked the compiler to inline it

js/src/xpconnect/src/nsXPConnect.cpp
line 972 -- *compartment = tempGlobal->getCompartment();

here someone tries to use it

they included 'jsobj.h' which opaquely speaking declares getCompartment
in normal stuff that's all you'd need, but because of the <inline> token, they need to randomly include <js*.h> to fish for whereever you hid the actual impl of getCompartment

Comment 3

[timeless]

iirc the approach layout/content take for stuff like this is to have two methods:
a private one and a public one.

the public one is not inline and out of line calls the inline private one.

Comment 4

[Jason Orendorff [:jorendorff]]

The patch is the minimal way to kill the warning, but if it's not performance-sensitive, we should just do what comment 3 says. The public one can be in jsapi.h.

Comment 5

[Andreas Gal :gal]

getCompartment() is entirely unnecessary. Lets just rip it out. There is already ->compartment().

Comment 6

[Jason Orendorff [:jorendorff]]

Agreed, but that leaves the warning.

I really don't care how we fix the warning as long as it gets fixed.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 494348 [details] [diff] [review]
i do not like this green eggs and ham

In this case, we definitely want everything inlined (especially compartment(), where the function call overhead will easily overwhelm the overhead of actually doing the computation.

timeless, would you mind s/getCompartment/compartment/g before landing?

Comment 8

[timeless]

Attachment: replace getCompartment() with compartment()

Comment 9

[Daniel Holbert [:dholbert]]

This bug's latest patch causes a 100%-reproducible shutdown segfault for me.  Just start up firefox, with this patch applied (using fresh profile, viewing about:blank), and then quit.  --> Segfault.

I've hit this both in libxul & non-libxul debug builds.

Comment 10

[Daniel Holbert [:dholbert]]

The crash from comment 9 is infinite recursion in js memory-freeing code.  It's a repeating pattern of two methods ping-ponging back and forth:
> #0  0x00007f6599d1959d in js::GCChunkAllocator::moz_free (this=0x7f659a73ff80, chunk=0x7f6589200000) at jsgcchunk.h:76
> #1  0x00007f6599d1a038 in XPConnectGCChunkAllocator::doFree 
(this=0x7f659a73ff80, chunk=0x7f6589200000) at xpcjsruntime.cpp:1182
> #2  0x00007f6599d1959f in js::GCChunkAllocator::moz_free (this=0x7f659a73ff80, chunk=0x7f6589200000) at jsgcchunk.h:76
> #3  0x00007f6599d1a038 in XPConnectGCChunkAllocator::doFree (this=0x7f659a73ff80, chunk=0x7f6589200000) at xpcjsruntime.cpp:1182
[etc]

Comment 11

[timeless]

Comment on attachment 495832 [details] [diff] [review]
replace getCompartment() with compartment()

:(

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 494348 [details] [diff] [review]
i do not like this green eggs and ham

The same is true for the first attachment.

Comment 13

[Brendan Eich [:brendan]]

New patch coming?

/be

Comment 14

[timeless]

nope, if just including jsgc.h triggers recursion then that's way too much magic for me at this time.

i've switched to a coverity crusade. i have 1400 items to review and a week to try to do them.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for the recurring to death

It turns out there's magic pixie dust to spray here. I also moved the js headers above the mozilla headers to avoid this problem in xpcpublic.h, but added the pixie dust anyway in order to allow arbitrary nesting between xpcpublic.h and other mozilla headers (the interdiff between this patch and attachment 495832 [details] [diff] [review] is only in xpcpublic.h, jsobj.h, jscntxt.h and jsgc.h.

Comment 16

[Andreas Gal :gal]

Comment on attachment 502982 [details] [diff] [review]
Fix for the recurring to death

Wow this is disgusting. There has to be a better way.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #16)
> Wow this is disgusting. There has to be a better way.

You are *so* welcome to investigate it. Until then, this is less ugly than the warnings (and potential problems).

Comment 18

[Andreas Gal :gal]

Yeah, I didn't r- it but wow is this ugly. Luke to the rescue after ff4?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/c098265c18e2

Comment 20

[Paul Wright]

Should there be a follow-up bug filed, per comment 18?

Comment 21

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/c098265c18e2

Comment 22

[Daniel Holbert [:dholbert]]

This doesn't appear to be completely fixed.  Filed new bug (Bug 630306) at mrbkap's request.

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/tracemonkey/rev/c05d5ba43182
http://hg.mozilla.org/mozilla-central/rev/c05d5ba43182

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #20)
> Should there be a follow-up bug filed, per comment 18?

I filed bug 640793.