Closed Bug 1034463 Opened 7 years ago Closed 7 years ago

More crashes when using SavedStacks


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bzbarsky, Assigned: fitzgen)




(1 file, 5 obsolete files)

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

Relevant logs: -- this is showing a getLocation call trying to resize the hashtable, hence MarkUnbarriered<JSScript>, mark its kids and we die on 0x4b4b4b4b. -- 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.
Blocks: 857648
Assignee: nobody → nfitzgerald
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.
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.
Attached patch more-saved-stack-crashes.patch (obsolete) — Splinter Review
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?
Attachment #8453430 - Flags: feedback?(bzbarsky)
Comment on attachment 8453430 [details] [diff] [review]

Try run without this patch:

And with:

Looks like the crashes are gone!
Attachment #8453430 - Flags: feedback?(bzbarsky) → feedback+
Attachment #8453430 - Flags: review?(shu)
Comment on attachment 8453430 [details] [diff] [review]

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.
Attachment #8453430 - Flags: review?(shu)
Oops, step 2 above should be assertSameCompartment(cx, this, script);
Attached patch more-saved-stack-crashes.patch (obsolete) — Splinter Review
Fixed up as you asked in review.

New try push:
Attachment #8453430 - Attachment is obsolete: true
Attachment #8454577 - Flags: review?(shu)
Comment on attachment 8454577 [details] [diff] [review]

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.
Attachment #8454577 - Flags: review?(shu) → review+
It turns out that just adding this assertion is enough to allow existing tests to trip up the error case. In particular:


both gather cross-compartment stacks.
Forgot to refresh the patch. Here's the real one.
Attachment #8454745 - Attachment is obsolete: true
Attached patch more-saved-stack-crashes.patch (obsolete) — Splinter Review
New try push that should fix those compilation issues:
Attachment #8454577 - Attachment is obsolete: true
Attachment #8454749 - Attachment is obsolete: true
Attachment #8454929 - Flags: review+
Ack, forgot to wrap CompartmentChecker::check(SavedStacks *) in an ifdef JS_CRASH_DIAGNOSTICS.
Attachment #8454929 - Attachment is obsolete: true
Attachment #8454932 - Flags: review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.