673760 - Avoid clearing ArenaHeader::chunk before finalizing all GC things so it can be used in asserts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

The bug mentioned in bug 671702 comment 21 is a preexisting one. In particular, we clear ArenaHeader::compartment right after the arena becomes empty before the GC finishes finalizing all arenas. This means that during a compartment GC IsAboutToBeFinalized is going to return false for any thing from the arena due to a compartment mismatch.

For now I close the bug as potentially it may lead to accessing deleted memory.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch preserve ArenaHeader::compartment during the GC. To distinguish empty unallocated arenas during the conservative stack scan the patch sets ArenaHeader::thingKind for such arenas to FINALIZE_LIMIT.

Comment 2

[Gregor Wagner [:gwagner]]

In IsAboutToBeFinalized we have

    JSCompartment *thingCompartment = reinterpret_cast<const Cell *>(thing)->compartment();
    JSRuntime *rt = cx->runtime;
    JS_ASSERT(rt == thingCompartment->rt);

Shouldn't we see a thingCompartment == NULL for every GC (global and comp)? Are we just lucky that it never happens?

Comment 3

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

I'm a little confused here too. I thought we were supposed to be structuring GCs so that if some function checked IsAboutToBeFinalized(x), then we could guarantee that x definitely hasn't been swept yet. All the IsAboutToBeFinalized calls happen in weak marking, js_SweepAtomState, or JSCompartment::sweep, and these are all run before we sweep any arenas.

I guess we do expose IsAboutToBeFinalized through JSAPI. Is this where the problem is happening?

Comment 4

[Gregor Wagner [:gwagner]]

(In reply to comment #3)
> I'm a little confused here too. I thought we were supposed to be structuring
> GCs so that if some function checked IsAboutToBeFinalized(x), then we could
> guarantee that x definitely hasn't been swept yet. All the
> IsAboutToBeFinalized calls happen in weak marking, js_SweepAtomState, or
> JSCompartment::sweep, and these are all run before we sweep any arenas.
> 
> I guess we do expose IsAboutToBeFinalized through JSAPI. Is this where the
> problem is happening?

The JSGC_MARK_END callback should be fine.
I guess the JSGC_FINALIZE_END callback is the problem? Well just from the naming, IsAboutToBeFinalized shouldn't be called after/during JSGC_FINALIZE_END.

Comment 5

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

I just checked all the IsAboutToBeFinalized uses in mxr. They all look fine except for one in XPCWrappedNative::FlatJSObjectFinalized. That one is only called in debug builds, but there's nothing to guarantee that the object you're calling it on hasn't already been swept, as far as I can see. We could probably just take it out.

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> I just checked all the IsAboutToBeFinalized uses in mxr. They all look fine
> except for one in XPCWrappedNative::FlatJSObjectFinalized.

You are right, I should have checked xpconnect better than relining on the memory. So there is no bug except that one in debug-only xpc. I suppose we can require that IsAboutToBeFinalized can only be called before the finalization itself begins. But that is for another bugs.

> That one is only
> called in debug builds, but there's nothing to guarantee that the object
> you're calling it on hasn't already been swept, as far as I can see. We
> could probably just take it out.

mrbkap probably now the reason behind the assert...

Gregor, could you still review the patch as it helps to minimize those small-per-compartment GC chunk patches?

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to comment #6)
> 
> Gregor, could you still review the patch as it helps to minimize those
> small-per-compartment GC chunk patches?

I don't see a connection here. Can you explain why this helps?

Comment 8

[Gregor Wagner [:gwagner]]

And also why do you think this patch makes more sense?

Comment 9

[Igor Bukanov]

(In reply to comment #7)
> I don't see a connection here. Can you explain why this helps?

For the bug 671702 I need to move the compartment pointer from ArenaHeader:: to ChunkInfo::. As such it is necessary to use something to denote unallocated arena instead of null compartment pointer. But this is also useful on its own for this bug as it allows to keep the compartment pointer as is after marking the arena as unallocated. So this patch replaces the !compartment check with thingKind == FINALIZE_LIMIT minimizing the amount of changes in the bug 671702.

Comment 10

[Gregor Wagner [:gwagner]]

Comment on attachment 548019 [details] [diff] [review]
v1


>+#ifdef DEBUG
>+        memset(a, ArenaSize, JS_FREE_PATTERN);
>+#endif
>+        *prevp = &a->aheader;
>+        a->aheader.initAsNotAllocated();

Please unify the setting with a single function.
something like setNotAllocated()?

>+    /*
>+     * We do not clear aheader->compartment so IsAboutToBeFinalized can use
>+     * the field until finalization finishes.
>+     */
>+    aheader->thingKind = FINALIZE_LIMIT;
>     aheader->next = info.emptyArenaListHead;




>+
>+    /*
>+     * We do not clear aheader->compartment so IsAboutToBeFinalized can use
>+     * the field until finalization finishes.
>+     */
>+    aheader->thingKind = FINALIZE_LIMIT;

I don't think this comment is true now.

Another comment in AnreaHeader describing the new meaning of thingKind and compartment would be good.

Comment 11

[Igor Bukanov]

http://hg.mozilla.org/integration/mozilla-inbound/rev/862f7431e744

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/862f7431e744