1. Open Firefox to blank page 2. Open Venkman 3. Turn on profiling 4. Navigate to cnn.com (almost anything will work I think) 5. crash Looks like Collection and GC are involved but might just be venkman at fault as well.

It looks like code called from Venkman's GC callback for GC_END ends up trying to access the global object of a JSContext, which we now null out during cycle collection. I wonder if we could move the nulling of the global objects to GC_BEGIN and the resetting to after GC_MARK_END and cycle collection.

Haven't tested this patch yet, will do that tonight.

Comment on attachment 287308 [details] [diff] [review] v1 This won't working. Working on a better patch?

(In reply to comment #3) > (From update of attachment 287308 [details] [diff] [review]) > This won't working. Working on a better patch? I think it means that Venkman access the context for which the global object was collected. In that case it a simple fix would be to check for global object in sJSContext::DOMBranchCallback from mozilla/dom/src/base/nsJSEnvironment.cpp and quit the callback if the global is null.

(In reply to comment #4) > I think it means that Venkman access the context for which the global object > was collected. And if this true, it means that there is a bug in Venkman like a missing addref to the global object.

Yet another strange thing. Consider that nsJSContext::DOMBranchCallback code from mozilla/dom/src/base/nsJSEnvironment.cpp that triggers the null pointer access: if (callbackCount == INITIALIZE_TIME_BRANCH_COUNT_MASK + 1 && LL_IS_ZERO(ctx->mBranchCallbackTime)) { // Initialize mBranchCallbackTime to start timing how long the // script has run ctx->mBranchCallbackTime = PR_Now(); ctx->mIsTrackingChromeCodeTime = ::JS_IsSystemObject(cx, ::JS_GetGlobalObject(cx)); return JS_TRUE; } The code assumes that one can use JS_GetGlobalObject(cx) to decide if this is tracking chrome. But this is wrong since when the native stack contains nsXPCWrappedJSClass::CallMethod (like the case of this bug), the context can be arbitrary and its global has nothing to with the execution of the script. Instead of using ::JS_GetGlobalObject(cx) the code should do something like JSObject* obj = ::JS_GetScopeChain(cx); ctx->mIsTrackingChromeCodeTime = obj && :JS_IsSystemObject(cx, ::JS_GetGlobalForObject(cx, obj)); In this way there is no dependency on the global object.

(In reply to comment #6) > Yet another strange thing. Consider that nsJSContext::DOMBranchCallback code > from mozilla/dom/src/base/nsJSEnvironment.cpp that triggers the null pointer > access I was wrong in that comment. The code is correct under assumption that the chrome code will invoke only trusted scripts so it does not matter if the scripts are invoked directly or indirectly via entering xpconnect API. Still the suggest cure should work and solve the problem.

(In reply to comment #4) > I think it means that Venkman access the context for which the global object > was collected. No, JS_GetContextPrivate would have returned null.

This makes sure that the globals are only cleared after calling the chained callbacks for JSGC_BEGIN and that they are reset before calling the chained callbacks for JSGC_END. Seems to fix this bug and avoids similar bugs that might lurk in other code. I also made the hashtable stick around, it seems like a waste to create/destroy it every time cycle collection runs.

Comment on attachment 287325 [details] [diff] [review] v2 I should probably add a check to the beginning of RestoreContextGlobals to return early if mClearedGlobalObjects.entryCount is 0.

I was able to generate this crash today, surfing around on ESPN.com. Stack trace: http://crash-stats.mozilla.com/report/index/80c7cdc0-8b3d-11dc-b37e-001a4bd46e84 Sorocco has it lasted as #50 top crashes. http://crash-stats.mozilla.com/report/list?range_unit=weeks&version=Firefox%3A3.0a9pre&range_value=2&signature=JS_IsSystemObject Perhaps a regression between after the 20071101 nightly builds? Crashes started happening after that date: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&date=hours&hours=2&mindate=2007-11-01&maxdate=2007-11-02&cvsroot=%2Fcvsroot

Definitely a 1.9 blocker. Leaving for drivers to say it's a beta blocker, but I'd say it is.

If we've got a fix - let's get it for beta...

Comment on attachment 287325 [details] [diff] [review] v2 Must have hit the wrong control, resetting r=igor.

peterv landed this at 2007-11-05 01:12.

Checked in with the change from comment 10.

verified fixed 1.9.0 2007110504 linux no crash. I get an "error throwing an exception" in the console trying to display profiled data but not when saving, but that is probably venkman.