888898 - Assertion failure: isEmpty(), at mozilla/LinkedList.h:267 with setObjectMetadataCallback and Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision d7553251cf43 (run with --fuzzing-safe --ion-eager):


var dbg = Debugger();
setObjectMetadataCallback(function() {});

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack for second testcase

x = Debugger()

asserts js debug shell on m-c rev d87b950c7a6f with -D when the testcase is passed in as a CLI argument at Assertion failure: isEmpty(), at dist/include/mozilla/LinkedList.h too.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/30c5b9ee2bc5
user:        Brian Hackett
date:        Mon Jul 08 09:17:35 2013 -0600
summary:     Bug 890636 - Remove JSOPTION_PCCOUNT, r=jandem.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Brian, is bug 890636 a likely regressor?

Comment 4

[Brian Hackett [Laid off!]]

The assertion is that there are still debuggers around when the runtime is destroyed.  I don't know what is supposed to ensure these debuggers get destroyed, but they are being marked through the global object since they are properties there.  In any case this doesn't have anything to do with my patches.

Comment 5

[Christian Holler (:decoder)]

Jason, any idea here? :)

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> The assertion is that there are still debuggers around when the runtime is
> destroyed.  I don't know what is supposed to ensure these debuggers get
> destroyed, but they are being marked through the global object since they
> are properties there.  In any case this doesn't have anything to do with my
> patches.

This is a symptom of bug 716981.

In short, we leak *everything* when a runtime is destroyed, Debuggers along with everything else. Obviously that's ... not great, and one result is this assertion.

We could treat the symptom, just clearing the list in JSRuntime::~JSRuntime.

Or fix the problem, by fixing bug 716981.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Should we then reopen that bug? billm marked it WONTFIX, so setting needinfo for him here.

Comment 8

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

Attachment: profiling

As far as I can tell, there's nothing that will cause JSScript::hasScriptCounts to be cleared before shutdown. We unconditionally mark all scripts with this flag set. This patch just makes shutdown GCs special so that they're not preserved.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc5dadbcb78e

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bc5dadbcb78e

Comment 11

[Christian Holler (:decoder)]

This is not fixed. The testcase in comment 0 still reproduces.

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision 9b4c4e56f4bb).

Comment 13

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

Attachment: metadata-fix

Sorry, I only tested with what Gary posted in comment 2.

This patch should fix the other issue. We're adding a root that never gets removed. This patch puts the metadata callback on the global rather than in a global variable.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/716d1f856bdf

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/716d1f856bdf