402379 - Crash with Venkman profiling [@ JS_IsSystemObject]

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: stack

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.

Comment 1

[Peter Van der Beken [:peterv]]

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.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

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

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 287308 [details] [diff] [review]
v1

This won't working. Working on a better patch?

Comment 4

[Igor Bukanov]

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

Comment 5

[Igor Bukanov]

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

Comment 6

[Igor Bukanov]

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.  

Comment 7

[Igor Bukanov]

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

Comment 8

[Peter Van der Beken [:peterv]]

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

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v2

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 10

[Peter Van der Beken [:peterv]]

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.

Comment 11

[Tony Chung [:tchung]]

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

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 13

[Mike Schroepfer]

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

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 287325 [details] [diff] [review]
v2

Must have hit the wrong control, resetting r=igor.

Comment 15

[Reed Loden [:reed]]

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

Comment 16

[Peter Van der Beken [:peterv]]

Checked in with the change from comment 10.

Comment 17

[Bob Clary [:bc] (inactive)]

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.