684575 - [jsdbg2] GC'ing a Debugger instance causes JSCompartment::updateForDebugMode to try to use CellIter during GC

Status: RESOLVED WORKSFORME

(Core Graveyard :: Java to XPCOM Bridge, defect)

Description

[Christian Holler (:decoder)]

The following test asserts on mozilla-central revision a351ae35f2c4 (options -m -n):

var g = newGlobal('new-compartment');
var dbg = Debugger(g);
(dbg) = function (f) {}
try {
  a = new Array();
  while (1) a = new Array(a);
} catch(e) {}


I assume this is jsdbg2 API only.

Comment 1

[Christian Holler (:decoder)]

Note: The specified mozilla-central revision had a build problem with standalone shell, patch from http://hg.mozilla.org/integration/mozilla-inbound/rev/fff3dc9478ce fixes this.

Comment 2

[Jim Blandy :jimb]

This bug stopped appearing with the changeset below, but I really doubt that addressed the underlying issue.

changeset:   77155:9208ee94b012
user:        Bill McCloskey <[e-mail]>
date:        Mon Sep 19 15:22:31 2011 -0700
summary:     Bug 604747 - Set GC max heap size to 4GB in JS shell (r=gregor)

Comment 3

[Jim Blandy :jimb]

Here's what's going on.

A Debugger is GC'd. Thus, each of its debuggees can be taken out of debug mode. However, JSCompartment::updateForDebugMode tries to use CellIter to search for affected scripts --- but the use of CellIter is forbidden during garbage collection.

Comment 4

[Jim Blandy :jimb]

There are some debugger/script analysis interactions here I don't understand.

If we're in an activeAnalysis, and do a GC that collects Debuggers, compartments could come out of debug mode. That invalidates some script analyses, but activeAnalysis means that someone is actively using that information. Do we need to queue the debug mode change until the analysis is complete?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I was just looking at the CellIter/CellIterUnderGC code, and it seems pretty error-prone. Would it be possible to have a single class that works regardless of whether a GC is happening (by checking the gcRunning flag)?

Also, it seems like CellIter requires background finalization to have completed. It doesn't wait for it. But I don't see any reason why the callers (like updateForDebugMode) can guarantee that. Do you know what's going on here, Igor?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bill McCloskey (:billm) from comment #5)
> I was just looking at the CellIter/CellIterUnderGC code, and it seems pretty
> error-prone. Would it be possible to have a single class that works
> regardless of whether a GC is happening (by checking the gcRunning flag)?

Since incremental GC landed, it should be safe to use CellIter from a GC (due to some code simplifications that cause us to keep the free list synced during the GC).

> Also, it seems like CellIter requires background finalization to have
> completed. It doesn't wait for it. But I don't see any reason why the
> callers (like updateForDebugMode) can guarantee that. Do you know what's
> going on here, Igor?

Igor explained to me a while back that this isn't a problem because we only use CellIter for AllocKinds that can't be finalized in the background (and we assert this).

It sounds like Jim's question is comment 4 is still a possible problem, though.

Comment 8

[Jim Blandy :jimb]

The restrictions on js::gc::CellIter's use have relaxed since this bug was filed. In particular, it's okay to use during GC, and scripts are never finalized in the background. (And CellIter asserts that you only iterate over things that won't be finalized in the background.)