639954 - TM: calling SwitchToCompartment(NULL) does nothing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

For this code in GCUntilDone:

    /*
     * We should not be depending on cx->compartment in the GC, so set it to
     * NULL to look for violations.
     */
    SwitchToCompartment(cx, (JSCompartment *)NULL);

SwitchToCompartment is a class, and when class constructors are called as functions the destructor is called immediately afterwards (thanks, C++!), so the original context's usually-bogus compartment is restored for the GC.

Comment 1

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

Oops. We have checks to ensure that this doesn't happen, but I guess they aren't called by SwitchCompartment. We should fix that.

Comment 2

[Brian Hackett [Laid off!]]

Hmm, this was the only hit for SwitchCompartment being called as a function.  Unfortunately, doing the obvious fix for this case broke some jit-tests in non-obvious ways, going to disown.

Comment 3

[Andreas Gal :gal]

Wow, nice. Ok, we need an owner and fixing. Gregor? Bill? Anyone?

Comment 4

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

I've only looked at one failure so far, but here's what I think is happening. In MarkRuntime, we iterate over all contexts cx and mark cx->compartment. NULLing-out cx->compartment during GC causes us to miss a compartment.

Once bug 639270 lands, this shouldn't be a problem. Let's wait for that and then fix this.

Comment 5

[Luke Wagner [:luke]]

If you add the idiomatic JS_GUARD_OBJECT_NOTIFIER* magic (grep for examples), then you will get a debug assert running the statement in comment 0.  (In theory, we should be sprinkling this magic on all guard objects to prevent this category of errors.)

Comment 6

[Brendan Eich [:brendan]]

The verb-style name fooled me into thinking this was an overloaded function, not a class. Use noun phrases for classes and structs, please!

/be

Comment 7

[Luke Wagner [:luke]]

I didn't really like it at first, but the pattern seems to be an Auto* prefix.

Comment 8

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

Attachment: fix

This fixes the GC case and adds the necessary guards to SwitchToCompartment. Now that bug 639270 has landed, there are no more test failures.

I didn't do any renaming. Should I?

Comment 9

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

http://hg.mozilla.org/tracemonkey/rev/655c1169d04b

Comment 10

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

Filed bug 641558 on the renaming.

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/655c1169d04b