Closed Bug 402379 Opened 12 years ago Closed 12 years ago

Crash with Venkman profiling [@ JS_IsSystemObject]

Categories

(Core :: XPConnect, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: bc, Assigned: peterv)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file 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.
Flags: in-testsuite-
Flags: in-litmus-
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.
Assignee: general → nobody
Blocks: 401687
Severity: normal → critical
Component: JavaScript Engine → XPConnect
Flags: blocking1.9?
Keywords: regression
OS: Linux → All
QA Contact: general → xpconnect
Hardware: PC → All
Attached patch v1 (obsolete) — Splinter Review
Haven't tested this patch yet, will do that tonight.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Comment on attachment 287308 [details] [diff] [review]
v1

This won't working. Working on a better patch?
Attachment #287308 - Attachment is obsolete: true
(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.
Attached patch v2Splinter Review
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.
Attachment #287325 - Flags: review?(igor)
Target Milestone: --- → mozilla1.9 M9
Attachment #287325 - Flags: review?(igor) → review+
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.
Attachment #287325 - Flags: review+ → review?(igor)
Definitely a 1.9 blocker. Leaving for drivers to say it's a beta blocker, but I'd say it is.
Flags: blocking1.9? → blocking1.9+
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.
Attachment #287325 - Flags: superreview?(jst)
Attachment #287325 - Flags: review?(igor)
Attachment #287325 - Flags: review+
peterv landed this at 2007-11-05 01:12.
Attachment #287325 - Flags: superreview?(jst) → superreview+
Checked in with the change from comment 10.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Crash Signature: [@ JS_IsSystemObject]
You need to log in before you can comment on or make changes to this bug.