624168 - inconsistency in crossCompartment wrapper data structure

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gregor Wagner [:gwagner]]

Enabling the per-compartment GC requires a consistent cross compartment information but currently there is somewhere a bug that results in a crash if I completely rely on this information. This behavior is not introduced with this patch.

The issue shows up in the IsAboutToBeFinalized function during running the browser reftests in debug and optimize build. 
With the new per-compartment GC option we have to know whether we are currently performing a per-compartment GC or not in this function. This information is stored in the runtime (gcCurrentCompartment). The runtime pointer is stored in the compartment and each object within the compartment can access this information. Somewhere on the way to the runtime pointer we get lost and the runtime pointer we get is bogus.
The inconsistency shows up during compartment sweeping.

In order to reproduce, just add an assertion JS_ASSERT(rt == thingCompartment->rt); in the IsAboutToBeFinalized function and run the browser reftests.


#0  0x0000000101a6b5de in JS_Assert (s=0x101f5570f "rt == thingCompartment->rt", file=0x101f54760 "/users/idefix2/moz/ws7/js/src/jsgc.cpp", ln=495) at /users/idefix2/moz/ws7/js/src/jsutil.cpp:80
#1  0x0000000101963873 in IsAboutToBeFinalized (cx=0x105b4f6a0, thing=0x124bacf30) at /users/idefix2/moz/ws7/js/src/jsgc.cpp:495
#2  0x0000000101917f47 in JSCompartment::sweep (this=0x131b98e00, cx=0x105b4f6a0, releaseInterval=0) at /users/idefix2/moz/ws7/js/src/jscompartment.cpp:393
#3  0x000000010195f174 in SweepCompartments (cx=0x105b4f6a0, gckind=GC_NORMAL) at /users/idefix2/moz/ws7/js/src/jsgc.cpp:2082
#4  0x0000000101962f81 in MarkAndSweep (cx=0x105b4f6a0, gckind=GC_NORMAL, gcTimer=@0x7fff5fbfcd40) at /users/idefix2/moz/ws7/js/src/jsgc.cpp:2253
#5  0x000000010196315e in GCUntilDone (cx=0x105b4f6a0, gckind=GC_NORMAL, gcTimer=@0x7fff5fbfcd40) at /users/idefix2/moz/ws7/js/src/jsgc.cpp:2502

Comment 1

[Andreas Gal :gal]

I am trying to catch this by adding assertions and printfs. Gregor is trying out VMWare Workstation for Linux and reverse debugging. bent is trying the windows replay box (which is still broken, most likely).

Comment 2

[Gregor Wagner [:gwagner]]

I might have an idea about the cause.

For a full GC we don't mark the content of the crossCompartment maps.
When we sweep the compartments in SweepCompartments we iterate over all compartments and delete unmarked ones right away. A following compartment could have a pointer in the crossCompartment vector that points into the previous deleted compartment. When we sweep the compartment, we iterate over this vector and follow these pointers. So we end up following a just finalized object to a just deleted compartment.

I guess we have to iterate over the compartments twice. First, sweep the crossCompartment vector and then iterate over it again and delete unused compartments.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I can't get reftests to assert in a debug windows build...

Comment 4

[Gregor Wagner [:gwagner]]

We are using pointers to just finalized objects. So lets close this bug.

SweepCompartments is called after we finalize all arenaLists.
This means we call IsAboutToBeFinalized with pointers to already finalized objects.

We can't call SweepCompartments before calling finalizeArenaLists because otherwise we would delete the compartments before we finalize their arenalists. Chicken and egg problem....

I guess the solution is to split up the SweepCompartments function.
First we have to sweep the crossCompartment map, then we can finalize all objects and afterwards we can iterate over all compartments and delete  unmarked compartments.

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: v1

Comment 6

[Gregor Wagner [:gwagner]]

Attachment: v2

Comment 7

[Andreas Gal :gal]

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

Awesome job. Thanks a lot!

Comment 8

[Gregor Wagner [:gwagner]]

http://hg.mozilla.org/tracemonkey/rev/43fcf9a72eb3

Comment 9

[Brendan Eich [:brendan]]

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

>     /*
>      * Figure out how much JIT code should be released from inactive compartments.
>      * If multiple eighth-lifes have passed, compound the release interval linearly;

s/lifes/lives/

>      * if enough time has passed, all inactive JIT code will be released.
>      */
>     uint32 releaseInterval = 0;
>     int64 now = PRMJ_Now();
>     if (now >= rt->gcJitReleaseTime) {
>         releaseInterval = 8;
>         while (now >= rt->gcJitReleaseTime) {
>             if (--releaseInterval == 1)
>                 rt->gcJitReleaseTime = now;
>             rt->gcJitReleaseTime += JIT_SCRIPT_EIGHTH_LIFETIME;
>         }
>     }
> 
>+    /* Remove dead wrappers from the compartment map. */
>+    for (JSCompartment **c = rt->compartments.begin(); c != rt->compartments.end(); ++c) {
>+        (*c)->sweep(cx, releaseInterval);
>+    }

Nit: don't brace.

>+    

Nit: trailing whitespace and useless blank line.

>     while (read < end) {
>         JSCompartment *compartment = (*read++);

Nit: lose the unnecessary parens.

>         if (compartment->marked) {
>             compartment->marked = false;
>             *write++ = compartment;
>-            /* Remove dead wrappers from the compartment map. */
>-            compartment->sweep(cx, releaseInterval);
>         } else {
>             JS_ASSERT(compartment->freeLists.isEmpty());
>             if (compartment->arenaListsAreEmpty() || gckind == GC_LAST_CONTEXT) {

gckind condition is loop invariant -- specialize by hoisting?

>                 if (callback)
>                     (void) callback(cx, compartment, JSCOMPARTMENT_DESTROY);
>                 if (compartment->principals)
>                     JSPRINCIPALS_DROP(cx, compartment->principals);
>                 delete compartment;
>             } else {
>                 compartment->marked = false;
>                 *write++ = compartment;
>-                compartment->sweep(cx, releaseInterval);

Repeats the first if's consequent. Could make this common code at the bottom of the loop body, continue past it if (!compartment->marked && compartment->arenaListsAreEmpty()) after doing the callback and delete. This raises the question: if the compartment is marked but its arenaListsAreEmpty(), isn't something wrong? How could that happen?

The answer may be that other code is simpler if an empty compartment can be marked, but if not, then the condition guarding the early callback/DROP/delete/continue if-then is even simpler.

>     /*
>      * We finalize iterators before other objects so the iterator can use the
>      * object which properties it enumerates over to finalize the enumeration
>      * state. We finalize objects before other GC things to ensure that
>      * object's finalizer can access them even if they will be freed.
>      */
> 

Lose this blank line.

>     for (JSCompartment **comp = rt->compartments.begin(); comp != rt->compartments.end(); comp++)

It seems |c| is the canonical name for JSCompartment ** variables, suggest using it consistently. It pays off for readers and grep.

/be

Comment 10

[Andreas Gal :gal]

I want this in asap for tonights nightly, gregor can you please fix comment 9? I filed a bug with brendan's comments (bug 624224).

http://hg.mozilla.org/mozilla-central/rev/4a3866321a14