264577 - XPConnect GC holes and hazards

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Brendan Eich [:brendan]]

Timeless is finding these in his day job, one unlikely last-ditch GC at a time.
 To help him, I advised turning on TOO_MUCH_GC in jsobj.c and jsgc.c -- that
works all too well, and crashes Firefox (trunk) on startup (jst had a branch
Firefox go off into some weeds, maybe looping or thread-death, not sure).

Patches coming up, and I'd like to work fast and fix several problems in this
bug, so fasten your seatbelts!

These may explain some longstanding topcrash and talkback-reported crash bugs.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fixes, plus some debugging grot not for check-in

I'll post a cleaned-up version when ready for reviews.	For now, you *want*
this patch if you define TOO_MUCH_GC in jsgc.h.

DOM patch next.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: DOM fix, independent of XPConnect patch

<brendan> TOO_MUCH_GC is not TOTALLY_TOO_MUCH_GC, which sets rt->gcPoke before
calling js_GC from js_AllocGCThing
<brendan> all the time
<brendan> *that*, i just tried, and xpconnect frustrates it
<shaver> does that make it worse?
<shaver> ah
<brendan> jband has his own data structure mark/sweep GC
<shaver> yes
<brendan> and he sweeps only the XPCNativeInterfaces that he puts in the
XPCJSRuntime's mIID2NativeInterfaceMap
<brendan> but with my fixes to close real GC holes, he marks other interfaces
<brendan> and then assertions botch and the member count has a high (15th from
0) mark bit set in its member count, and stuff
<brendan> but, with just TOO_MUCH_GC, i can now bring up firefox
<brendan> this nests a last ditch if the gc has been poked
<brendan> on every alloc after a poke
<brendan> found jst being naughty too, with the recently added global scope
polluter (window.id hack) object
<brendan> it was flapping in the breeze
<brendan> in SetNewDocument
<shaver> nice finds
<shaver> wow
<brendan> patches coming
<shaver> that'll help us a lot
<shaver> I stand by to r!

Comment 3

[Brendan Eich [:brendan]]

Calling for early reviews, don't worry about the debugging hacks and printf in
xpcwrappednative.cpp.

Questions:
- Is there a way we can tell that the wrapper will be marked by its scope, if
it's in a scope's map, and avoid the JS_MarkGCThing on mFlatJSObject
(xpcprivate.h)? Not a big deal, but maybe worth avoiding that JS API call.
- Any better way to protect the new parent returned by PreCreate in
XPCWrappedNative::GetNewOrUsed?
- Suggestions on how to turn on TOTALLY_TOO_MUCH_GC (which would poke the GC
before every js_GC call from js_AllocGCThing)?  As noted in comment 2, the IRC
log part, this leads to XPConnect marking some XPCNativeInterfaces reachable
from wrapped natives (I think), but not yet in the runtime's
mIID2NativeInterfaceMap -- so not swept.  That leaves the mark bit set, which
botches assertions and makes the XPCNativeInterface's member count look huge.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: Add WAY_TOO_MUCH_GC ifdef to jsgc.c

WAY_, not TOTALLY_.

Note that if you define WAY_TOO_MUCH_GC that defines TOO_MUCH_GC, so this pain
is convenient pain.

/be

Comment 5

[Brendan Eich [:brendan]]

Many thanks to jst for bug-buddying and using a better debugger (gdb is *even
worse* in FC2!  WTF?!)

/be

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 162259 [details] [diff] [review]
DOM fix, independent of XPConnect patch

r+sr=jst

Comment 7

[timeless]

Comment on attachment 162258 [details] [diff] [review]
proposed fixes, plus some debugging grot not for check-in

+--xpc_nogcflag;
	 return rv;

this is asking for a macro or autoobject :)

Comment 8

[Brendan Eich [:brendan]]

Timeless: it's just a local hack, not worth the trouble because it may give
false alarms (nested GCs may collect true temporary garbage, although I'm not
sure yet that such garbage is not a bug).

/be

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 162259 [details] [diff] [review]
DOM fix, independent of XPConnect patch

a=me for branches, this is a clean fix.

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 162259 [details] [diff] [review]
DOM fix, independent of XPConnect patch

This patch has been checked into trunk and branches.

/be

Comment 11

[Brendan Eich [:brendan]]

There's more to patch here, and the attached patch (no comments from dbradley or
shaver yet) is *not* ready to land.

/be

Comment 12

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

This looks like it may have hurt Tp on btek a bit.

Comment 13

[Brendan Eich [:brendan]]

We looked harder and saw NSPR's linux.h changing, which caused a big recompile,
which may have reordered the build enough to hurt Tp.  We're not sure, but I am
not feeling guilty enough to back out.  Someone might convince me, or perhaps we
could back out locally on Btek?

/be

Comment 14

[David Bradley]

Comment on attachment 162258 [details] [diff] [review]
proposed fixes, plus some debugging grot not for check-in

r=dbradley

I went back to some of the objects graphs I did for GC and realized that the
flattened JSObject was no where to be found. So this does seem like a good idea
as does protecting that parent object.

Comment 15

[Brendan Eich [:brendan]]

Attachment: cleaned up patch

I'm worried that jband requires mFlatObject to be ephemeral if there are no
strong XPCOM refs on the wrapper, and that we'll have leaks with the
xpcprivate.h patch included here.

The xpcwrappednative.cpp patch is good, we need it to avoid death if a
last-ditch GC should nest while a parent revised by PreCreate is flapping in
the breeze.  I'm going to check just xpcwrappednative.cpp into branches and
trunk now, for Firefox 1.0 RC1.  I need more testing, thinking, and advice on
the xpcprivate.h patch.

/be

Comment 16

[Asa Dotzler [:asa]]

This bug seems to have an aviary branch checkin associated with it. If this has
landed on the aviary branch (as much as it's going to for 1.0) can you please
add the "fixed-aviary1.0" keyword? Thanks.

Comment 17

[Brendan Eich [:brendan]]

Not sure what to do, crave some advice from jband.

/be

Comment 18

[Scott MacGregor]

Hey Brendan,
I just started seeing some talkback reports using builds from 10/23 and later
involving JS garbage collection. could they be related to this change?

See:

http://hal.mozilla.org/reports/incidenttemplate.cfm?bbid=1576913

or it could just be coincidence...

Comment 19

[Brendan Eich [:brendan]]

Nothing more to do here without better ideas from dbradley and jband.

/be

Comment 20

[Scott MacGregor]

Hey Brendan, did you see my comments in comment #18 before you minused and moved
this off the 1.0 radar? 

Comment 21

[Ryan VanderMeulen [:RyanVM]]

brendan, is this bug still in need of more work or has 3 years of development taken care of things?

Comment 22

[Brendan Eich [:brendan]]

This bug is done. Comment 19 should have invited new bugs for new ideas.

/be