1034463 - More crashes when using SavedStacks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I tried checking in bug 857648 but got crashes in various places in SavedStacks code.

Relevant logs:

https://tbpl.mozilla.org/php/getParsedLog.php?id=43107644&tree=Mozilla-Inbound&full=1#error1 -- this is showing a getLocation call trying to resize the hashtable, hence MarkUnbarriered<JSScript>, mark its kids and we die on 0x4b4b4b4b.

https://tbpl.mozilla.org/php/getParsedLog.php?id=43107566&tree=Mozilla-Inbound&full=1#error1 -- this is showing GC calling sweepPCLocationMap which calls IsAboutToBeFinalized<JSScript> which then dies at address 0x2cc.

In both cases it seems like we ended up with dead JSScripts in pcLocationMap.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

For the first crash, it seems that in the process of resizing the table, we are reading the PreBarrieredScript in the key and this is what is triggering the marking. The two options I can think of to fix this would be to either a) always sweep the table and check for scripts that are about to be finalized before adding any new entries (can we call IsScriptAboutToBeFinalized when not under GC? If not, then I would expect all the scripts to be valid until the next GC, so why weren't they cleaned up in the previous sweep?) or b) hold the scripts strongly (which is definitely not ideal).

As to the second crash, I don't see how we could have bad script pointers there, as that is supposed to be the place where we clean up and remove entries for scripts that are being finalized. If there is a bad script in there, then it must have been somehow missed by the previous sweep. Maybe the sweep method isn't being called as often as it should be.

Not sure what's going on, continuing to dig.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok, so here is how I think we get dead scripts in the cache that don't get swept at the proper time: cross compartment scripts.

First, going to try and make a JS shell test case that reproduces these crashes.

Second, going to modify |insertFrames| to call the script's compartment's saved stack's getLocation rather than the current compartment's saved stack's getLocation. This way the invariant will be that the PCLocationMap will only ever hold scripts within its own compartment.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: more-saved-stack-crashes.patch

Unfortunately, I wasn't able to reproduce any crashes with the js shell. Pretty simple fix to ensure that we only ever store our own compartment's scripts in our PCLocationMap, though.

:bz, I'd have done a try push myself, but it seems you have quite the patch queue going, so perhaps you could test this patch out?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8453430 [details] [diff] [review]
more-saved-stack-crashes.patch

Try run without this patch: https://tbpl.mozilla.org/?tree=Try&rev=06c87ab0eba4

And with: https://tbpl.mozilla.org/?tree=Try&rev=8579ff830e86

Looks like the crashes are gone!

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8453430 [details] [diff] [review]
more-saved-stack-crashes.patch

Review of attachment 8453430 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/SavedStacks.cpp
@@ +620,5 @@
> +    // We should only ever be caching location values for scripts in this
> +    // compartment. Otherwise, we would get dead cross-compartment scripts in
> +    // the cache because our compartment's sweep method isn't called when their
> +    // compartment gets collected.
> +    JS_ASSERT(&script->compartment()->savedStacks() == this);

This ad hoc check doesn't follow our compartment conventions, I don't think. I'd rather that all the compartment checks are on the cx for consistency and readability. I suggest the following refactoring:

1. Give a SavedStacks specialization for assertSameCompartment.
2. Convert this check to assertSameCompartment(cx, this);
3. Actually enter the script compartment when calling getLocations in insertFrames:

{
    AutoCompartment(cx, compartment);
    if (!compartment->savedStacks().getLocation(...))
        return false;
}

What do you think?

Relatedly, after step 1 the existing JS_ASSERT(&cx->compartment()->savedStacks() == this); checks can be rewritten as assertSameCompartment(cx, this); as well.

Comment 6

[Shu-yu Guo [:shu]]

Oops, step 2 above should be assertSameCompartment(cx, this, script);

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: more-saved-stack-crashes.patch

Fixed up as you asked in review.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=1b4271e15269

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8454577 [details] [diff] [review]
more-saved-stack-crashes.patch

Review of attachment 8454577 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/SavedStacks.cpp
@@ +677,5 @@
> +void
> +CompartmentChecker::check(SavedStacks *stacks)
> +{
> +    JS_ASSERT(stacks);
> +    check(stacks->compartment());

You don't need to keep a pointer to the compartment in SavedStacks. You can just check for compartment like |compartment->savedStacks() == stacks| or something.

Comment 9

[Jim Blandy :jimb]

Attachment: Assert that the SavedStacks location cache does not contain inter-compartment references.

It turns out that just adding this assertion is enough to allow existing tests to trip up the error case. In particular:

    /home/jimb/moz/dbg/js/src/jit-test/tests/saved-stacks/principals-01.js
    /home/jimb/moz/dbg/js/src/jit-test/tests/saved-stacks/principals-02.js

both gather cross-compartment stacks.

Comment 10

[Jim Blandy :jimb]

Attachment: Minimal assertions for catching cross-compartment script references in the SavedStacks location lookup cache.

Forgot to refresh the patch. Here's the real one.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: more-saved-stack-crashes.patch

New try push that should fix those compilation issues: https://tbpl.mozilla.org/?tree=Try&rev=3be9d1b60dea

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: more-saved-stack-crashes.patch

Ack, forgot to wrap CompartmentChecker::check(SavedStacks *) in an ifdef JS_CRASH_DIAGNOSTICS.

https://tbpl.mozilla.org/?tree=Try&rev=090c8ff931cc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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