Brendan Eich [:brendan] Reporter
	Description • 19 years ago

dbaron and igor have pointed out how JSGC_MARK_END is taken to mean JSGC_FINALIZE_BEGIN by XPConnect, and dbaron also observed how XPConnect uses JSGC_MARK_END to do some last-minute marking.  With the new close phase (see bug 326466), this is not sound.

We need JSGC_MARK_BEGIN and JSGC_FINALIZE_BEGIN. These are easy to add, and the patch will hit jspubtd.h and jsgc.c, but I'm filing this bug against xpconnect to make sure the API requirements are hashed out fully and then met.

/be

	Brendan Eich [:brendan] Reporter
	Comment 1 • 19 years ago

Do we want JSGC_CLOSE_BEGIN and JSGC_CLOSE_END too?  We can reorganize the JSGCStatus enum freely at this point, since we are not preserving binary compatibility from js1.6 to js1.7.

Igor, could you please take this bug?  I'm taking long-deferred vacation next Tuesday for three weeks, mrbkap is covering many areas already, and you are GC guru already.  Thanks,

/be

	Igor Bukanov Assignee
	Comment 2 • 19 years ago

(In reply to comment #1)
> Do we want JSGC_CLOSE_BEGIN and JSGC_CLOSE_END too?

Any particular need for this since I think even JSGC_MARK_BEGIN is unnecessary? 

AFAICS the only reason why xpconnect needs something like JSGC_MARK_BEGIN is to properly deal with autoroots. And this is in turn necessary since there is no public API in SpiderMonkey to quickly root native stack temporaries. If such API would be provided, it would remove the single legitimate reason for JSGC_MARK_BEGIN unless one really wants to duplicate SpiderMonkey rooting machinery. 

> 
> Igor, could you please take this bug?

Done

	Brendan Eich [:brendan] Reporter
	Comment 3 • 19 years ago

(In reply to comment #2)
> (In reply to comment #1)
> > Do we want JSGC_CLOSE_BEGIN and JSGC_CLOSE_END too?
> 
> Any particular need for this since I think even JSGC_MARK_BEGIN is unnecessary?

Leaving existing API alone would be best, as usual.
 
> AFAICS the only reason why xpconnect needs something like JSGC_MARK_BEGIN is to
> properly deal with autoroots. And this is in turn necessary since there is no
> public API in SpiderMonkey to quickly root native stack temporaries. If such
> API would be provided, it would remove the single legitimate reason for
> JSGC_MARK_BEGIN unless one really wants to duplicate SpiderMonkey rooting
> machinery.

Fact is we have duplication in xpconnect/src already, from the old days.  Rather than reform that all at once, it might be better to add MARK_BEGIN and use it for the moment (Firefox 2, Gecko 1.8.1).

Were you thinking of building a public API on top of JSTempValueRooter?  That would help other parts of the code, IIRC -- bz recalls the bug I'm sure.

Thanks for taking this.

/be

	David Baron :dbaron:
	Comment 4 • 19 years ago

(In reply to comment #1)
> We can reorganize the
> JSGCStatus enum freely at this point, since we are not preserving binary
> compatibility from js1.6 to js1.7.

Too bad.  I think that's the only binary compatibility I need from the JS engine for my leak-monitor extension that's not also source-code compatibility.  So even if you can reorder, it would be nice not to.  :-)

	David Baron :dbaron:
	Comment 5 • 19 years ago

(In reply to comment #4)
> I think that's the only binary compatibility

Not quite only:  I use JSGC_END and JS_MAP_GCROOT_NEXT.

	Igor Bukanov Assignee
	Comment 6 • 19 years ago

(In reply to comment #3)
> Were you thinking of building a public API on top of JSTempValueRooter?  That
> would help other parts of the code, IIRC -- bz recalls the bug I'm sure.

I was thinking about API that matches current xpconnect logic to minimize changes there:

typedef struct JSAutoRooter JSAutoRooter;

struct JSAutoRooter {
    JSAutoRooter        *prev;
};

typedef void (*JSAutoRooterCallback)(JSContext *cx, JSAutoRooter *autoRooter);

extern JS_PUBLIC_API(void)
JS_SetAutoRooterCallback(JSContext *cx, JSAutoRooterCallback callbak);
    
extern JS_PUBLIC_API(void)
JS_AddAutoRoots(JSContext *cx, JSAutoRooter *autoRooter);

extern JS_PUBLIC_API(void)
JS_ReleaseAutoRoots(JSContext *cx, JSAutoRooter *autoRoot);

Then JSAutoRooter can be embedded into AutoMarkingPtr from xpcprivate.h and a callback would be defined that extracts AutoMarkingPtr from JSAutoRooter and calls AutoMarkingPtr.MarkBeforeJSFinalize assuming that evil looking and AFAICS unused MarkAfterJSFinalize() would be removed.

Such API is extremely lightweight since JS_AddAutoRoots/JS_RemoveAutoRoots would set a couple of pointer fields. But this is really tailored for C++ embeddings since with single global callback C embedding would need to implement its own dispatching logic to select exactly what it wants to do with JSAutoRooter.

	Brendan Eich [:brendan] Reporter
	Comment 7 • 19 years ago

(In reply to comment #5)
> (In reply to comment #4)
> > I think that's the only binary compatibility
> 
> Not quite only:  I use JSGC_END and JS_MAP_GCROOT_NEXT.

We can definitely not reorder, but it sounds like we are not going to fiddle with the GC callback API at all.

OTOH, if your extension used JSFunctionSpec, it would be out of luck already. It must not, from what you say. See bug 334261.

/be

	Igor Bukanov Assignee
	Comment 8 • 19 years ago

It turned out that xpconnect finalization is much more complex then I naively expected when proposed to provide API for fast autorooting. So here is a version that just adds JSGC_MARK_BEGIN callback with the following notes:

1. The patch preserves the numbering of JSGCStatus enum constants from jspubt.h for extra compatibility.

2. I replaced gcRunning and gcClosePhase in JSRuntime by single gcPhase which is a enum that spans over JSGCPhase defined in jsgc.h. It allowed to add an assert to js_MarkGCThing to insist that the function should only be called during marking phase. When convertion the code to use the new enum I have found that AddRoot/RemoveRoot do not allow to be called during the close phase. This patch fixes that.

3. JSContext.insideGCMarkCallback is removed as the finalization callback is not allowed to do any marking.

4. I ported XPCJSRuntime::GCCallback to use te new callback constants. The patch renames mThreadRunningGC to mThreadRunningFinalize to reflect the real nature of the flag. Currently only marking of autoroots is moved to JSGC_MARK_BEGING with the rest of code kept at JSGC_FINALIZE_BEGIN.

In the current form the patch causes an assert on browser startup at the newly added check in js_MarkGCThing that insists on the marking phase. The problem is that XPCWrappedNativeScope::FinishedMarkPhaseOfGC calls marking code at the very end of JSGC_FINALIZE_BEGIN (see stack trace bellow).

AFAICS this is wrong even without close phase patch since such marking is called after self->mWrappedJSMap->Enumerate(WrappedJSDyingJSObjectFinder, &data). Since  
WrappedJSDyingJSObjectFinder uses JS_IsAboutToBeFinalized, it may allow to finalize an object that is in fact reachable via a reference that would be marked  via later call to XPCWrappedNativeScope::FinishedMarkPhaseOfGC.

So how to deal with this?

The stack trace leading to the assert:
  
#3  0x00dd8a6d in JS_Assert (
    s=0xdf13c0 "cx->runtime->gcPhase == JSGC_MARK_PHASE", 
    file=0xdf125c "/home/igor/w/mozilla/js/src/jsgc.c", ln=1872)
    at /home/igor/w/mozilla/js/src/jsutil.c:57
#4  0x00d81c4a in js_MarkGCThing (cx=0x98e6298, thing=0x99d62c8)
    at /home/igor/w/mozilla/js/src/jsgc.c:1872
#5  0x00d57a29 in JS_MarkGCThing (cx=0x98e6298, thing=0x99d62c8, 
    name=0x9b4298 "XPCWrappedNative::mFlatJSObject", arg=0x0)
    at /home/igor/w/mozilla/js/src/jsapi.c:1895
#6  0x0099fabc in WrappedNativeJSGCThingMarker (table=0x9a228d0, 
    hdr=0x9a22940, number=0, arg=0x98e6298)
    at /home/igor/w/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp:271
#7  0x00d6a6f7 in JS_DHashTableEnumerate (table=0x9a228d0, 
    etor=0x99fa6a <WrappedNativeJSGCThingMarker>, arg=0x98e6298)
    at /home/igor/w/mozilla/js/src/jsdhash.c:674
#8  0x00958f1d in Native2WrappedNativeMap::Enumerate (this=0x9a20a80, 
    f=0x99fa6a <WrappedNativeJSGCThingMarker>, arg=0x98e6298)
    at /home/igor/w/mozilla/js/src/xpconnect/src/xpcmaps.h:212
#9  0x0099f9c6 in XPCWrappedNativeScope::FinishedMarkPhaseOfGC (cx=0x98e6298, 
    rt=0x98a4c08)
    at /home/igor/w/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp:288
#10 0x0097db4c in XPCJSRuntime::GCCallback (cx=0x98e6298, 
    status=JSGC_FINALIZE_BEGIN)
    at /home/igor/w/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:313

#11 0x025a3d56 in DOMGCCallback (cx=0x98e6298, status=JSGC_MARK_END)
    at /home/igor/w/mozilla/dom/src/base/nsJSEnvironment.cpp:2206
#12 0x00d83344 in js_GC (cx=0x98e6298, gcflags=0)
    at /home/igor/w/mozilla/js/src/jsgc.c:2418
#13 0x00d83899 in js_ForceGC (cx=0x98e6298, gcflags=0)
    at /home/igor/w/mozilla/js/src/jsgc.c:1950
#14 0x00d62702 in js_DestroyContext (cx=0x98e6298, gcmode=JS_FORCE_GC)
    at /home/igor/w/mozilla/js/src/jscntxt.c:361
#15 0x00d5987f in JS_DestroyContext (cx=0x98e6298)
    at /home/igor/w/mozilla/js/src/jsapi.c:942
#16 0x00983a25 in XPCJSContextStack::SetSafeJSContext (this=0x98e6080, 
    aSafeJSContext=0x9a1f1f8)
    at /home/igor/w/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp:242
#17 0x0098439b in nsXPCThreadJSContextStackImpl::SetSafeJSContext (
    this=0x98a9e48, aSafeJSContext=0x9a1f1f8)
    at /home/igor/w/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp:398
#18 0x02930a9d in nsAppShellService::SetXPConnectSafeContext (this=0x99cdff8)
    at /home/igor/w/mozilla/xpfe/appshell/src/nsAppShellService.cpp:119
#19 0x02930c31 in nsAppShellService::CreateHiddenWindow (this=0x99cdff8, 
    aAppShell=0x98a3d00)
    at /home/igor/w/mozilla/xpfe/appshell/src/nsAppShellService.cpp:193
#20 0x00a404b2 in nsAppStartup::CreateHiddenWindow (this=0x98e07c8)

	Igor Bukanov Assignee
	Comment 9 • 19 years ago

This is aversion of the previous patch that asserts outside marking phase in js_MarkGCThing only when marking not already marked thing. This is mostly to check if that extra marking after calling IsAboutToBeFinalized is just unnecessary extra phase or a bug that can be easily triggered.

Plus I added asserts to IsAboutToBeFinalized to check for finalized phase as the function should only be used in that phase.

	Igor Bukanov Assignee
	Comment 10 • 18 years ago

Here is an untested prototype to trace XPC runtime structures via newly introduced callback.

	Igor Bukanov Assignee
	Comment 11 • 18 years ago

Again not very well tested quilt patch to use the bugzilla as a backup.

As always, the biggest chalenge is to come up with a good name for a trace method that an embedding can define to trace custom roots or global data holding traceable things.

	David Baron :dbaron:
	Comment 12 • 18 years ago

One other comment here about the unsoundness of what XPConnect does (the marking done in XPCPerThreadData::MarkAutoRootsBeforeJSFinalize and XPCWrappedNativeScope::FinishedMarkPhaseOfGC):  Using the JSGC_MARK_END callback to do more marking (based on what's already marked) can only be sound if only a single GC callback does it.  If more than one does it, then the extra marking done by earlier callbacks won't take into account extra marking done by later callbacks.  I think this suggests that any code doing this should be replaced by mark callbacks that happen during the mark phase.

(It also scares me that XPCWrappedNativeScope::FinishedMarkPhaseOfGC does marking after we fill mWrappedJSToReleaseArray with things to release after the GC is done and other similar things.  I wonder if that explains any topcrashes.)

	David Baron :dbaron:
	Comment 13 • 18 years ago

(In reply to comment #12)
> (It also scares me that XPCWrappedNativeScope::FinishedMarkPhaseOfGC does
> marking after we fill mWrappedJSToReleaseArray with things to release after the
> GC is done and other similar things.  I wonder if that explains any
> topcrashes.)

Actually, I think it's not so bad, because all the collection-like things done here (except the very last one) are really just clearing of caches of lazily-instantiated things.

	Igor Bukanov Assignee
	Comment 14 • 18 years ago

The patch update that touches only xpconnect changes as the necessary JS API extension now lives in a patch fro bug 379146. I will provide a CVS diff later for a review after landing patches for bug 377751 and bug 379146.

	Igor Bukanov Assignee
	Comment 15 • 18 years ago

The patch makes sure that all tracing happens inside XPCJSRuntime::TraceJS which is now traceOp for extra runtime roots. 

In the patch I wrote:

 void
 XPCWrappedNativeScope::TraceJS(JSTracer* trc, XPCJSRuntime* rt)
 {
+
+    // Hold the lock until return...
+    // XXX is it relly necessary???
+    XPCAutoLock lock(rt->GetMapLock());
+

But I am not sure that locking here is necessary given that this is called during GC or tracing when only one thread can be inside the request.

	Brendan Eich [:brendan] Reporter
	Comment 16 • 18 years ago

Comment on attachment 263526 [details] [diff] [review]
Implementation v1


>+    // Hold the lock until return...
>+    // XXX is it relly necessary???
>+    XPCAutoLock lock(rt->GetMapLock());

You're right, this shouldn't be necessary during GC -- but tracing is a superset of GC. So should this lock be taken iff !IS_GC_MARKING_TRACER(trc)?

What invariants does an XPCJSRuntime::mMapLock protect?

/be

	Igor Bukanov Assignee
	Comment 17 • 18 years ago

(In reply to comment #16)
> (From update of attachment 263526 [details] [diff] [review])
> 
> >+    // Hold the lock until return...
> >+    // XXX is it relly necessary???
> >+    XPCAutoLock lock(rt->GetMapLock());
> 
> You're right, this shouldn't be necessary during GC -- but tracing is a
> superset of GC. So should this lock be taken iff !IS_GC_MARKING_TRACER(trc)?

None of tracing/former marking code does any locking. I suggest for that to continue to be so and require rather for the trace initiator to ensure the runtime locking in the same way as GC does. AFAICS there is no API currently to do something like that:

StartSerializedRuntimeAccess
do tracing
EndSerializedRuntimeAccess

Should I file a bug about that?



 
> What invariants does an XPCJSRuntime::mMapLock protect?
> 
> /be
> 

	Igor Bukanov Assignee
	Comment 18 • 18 years ago

(In reply to comment #16)
> What invariants does an XPCJSRuntime::mMapLock protect?

It ensures serialized access to various XPCJSRuntime maps and from a quick grep I am not sure that the mutators for the maps are always run inside BeginRequest. So maybe the lock is necessary after all. 

 

	Igor Bukanov Assignee
	Comment 19 • 18 years ago

This version adds fixme bug 380139 comments about unnecessary locking and replaces the bogus comments in XPCWrappedNativeScope::FinishedMarkPhaseOfGC about JSGC_END by the assert and comments about JSGC_MARK_END and JSGC_FINALIZE_END.

	Igor Bukanov Assignee
	Comment 20 • 18 years ago

(In reply to comment #19)
> Created an attachment (id=264229) [details]
> implementation v2

Note that despite the warning the bugzilla properly shows the diff between v2 and v1. 

	Igor Bukanov Assignee
	Comment 21 • 18 years ago

v2 had NS_ASSERT, not NS_ASSERTION and this version fixes that.

	Johnny Stenback (:jst)
	Comment 22 • 18 years ago

Comment on attachment 264258 [details] [diff] [review]
implementation v2b

r=jst

	Igor Bukanov Assignee
	Comment 23 • 18 years ago

Patch to commit with couple of comment text fixes for better English.

	Igor Bukanov Assignee
	Comment 24 • 18 years ago

I committed the patch from comment 23 to the trunk:

Waiting for Emacs...
Checking in js/src/xpconnect/src/xpcjsruntime.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp,v  <--  xpcjsruntime.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.213; previous revision: 1.212
done
Checking in js/src/xpconnect/src/xpcwrappednativescope.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp,v  <--  xpcwrappednativescope.cpp
new revision: 1.39; previous revision: 1.38
done