1031168 - Crashes when using SavedStacks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Test patch

Steps to reproduce:

1)  Apply the attached patch.
2)  Build the browser.
3)  Load https://bug932837.bugzilla.mozilla.org/attachment.cgi?id=8356109
4)  Reload it a few times

Crashes pretty reliably for me.

The reason we're crashing is that we end up with a stack like this:

#0  JSString::asAtom (this=0x11f2ccb68) at String.h:449
#1  0x0000000102b55064 in js::SavedFrame::getSource (this=0x11f2df160) at SavedStacks.cpp:148
#2  0x0000000102b54e99 in js::SavedFrame::HashPolicy::match (existing=0x11f2df160, lookup=@0x7fff5fbef700) at SavedStacks.cpp:100
#3  0x0000000102baec35 in js::detail::HashTable<js::ReadBarriered<js::SavedFrame*> const, js::HashSet<js::ReadBarriered<js::SavedFrame*>, js::SavedFrame::HashPolicy, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::match (e=@0x117cebae0, l=@0x7fff5fbef700) at HashTable.h:1183
#4  0x0000000102badc8a in js::detail::HashTable<js::ReadBarriered<js::SavedFrame*> const, js::HashSet<js::ReadBarriered<js::SavedFrame*>, js::SavedFrame::HashPolicy, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::lookup (this=0x11ac508a0, l=@0x7fff5fbef700, keyHash=3861274004, collisionBit=1) at HashTable.h:1205
#5  0x0000000102baee2a in js::detail::HashTable<js::ReadBarriered<js::SavedFrame*> const, js::HashSet<js::ReadBarriered<js::SavedFrame*>, js::SavedFrame::HashPolicy, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::lookupForAdd (this=0x11ac508a0, l=@0x7fff5fbef700) at HashTable.h:1518
#6  0x0000000102b87714 in js::HashSet<js::ReadBarriered<js::SavedFrame*>, js::SavedFrame::HashPolicy, js::SystemAllocPolicy>::lookupForAdd (this=0x11ac508a0, l=@0x7fff5fbef700) at HashTable.h:373
#7  0x0000000102b570ce in js::SavedStacks::getOrCreateSavedFrame (this=0x11ac508a0, cx=0x1003a0f60, lookup=@0x7fff5fbef700) at SavedStacks.cpp:508

and in frame 7 lookup.source is stamped over with 0x4b4b4b4b, which, according to firebot, is "the poison value (JS_SWEPT_TENURED_PATTERN) written over tenured heap things and arenas after they are swept".

Our actual crash is under js::SavedFrame::getSource, but that's because _that_ atom is also swept.

I see nothing in sweepPCLocationMap that handles the "source" atoms the hashtable entries in pcLocationMap store going away, nor anything in SavedStacks::sweep for the "frames" hashtable.

We need to either add some IsStringAboutToBeFinalized checks, or move the source atom into wherever JSScript stores its filename right now, so the JSScript will just keep it alive.

One thing that confuses me here: how can SavedFrame::getSource be dead, if the SavedFrame is not?  I would have thought it would keep the atom alive, since it's in the slot...

Maybe we added it with an already-dead atom that we got out of the pcLocationMap?

Comment 1

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

(In reply to Boris Zbarsky [:bz] from comment #0)
> One thing that confuses me here: how can SavedFrame::getSource be dead, if
> the SavedFrame is not?  I would have thought it would keep the atom alive,
> since it's in the slot...

Yes, this is what I'd expected to happen. Surprising.

> Maybe we added it with an already-dead atom that we got out of the
> pcLocationMap?

Ah, yes we probably need to trace the atoms in that map.

Investigating...

Comment 2

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

Attachment: trace-sources.patch

This fixes the crash triggered by bz's test patch, for me. Unfortunately, I wasn't able to make a more reduced case.

Seems pretty straightforward in retrospect, though.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a3acad60dce1

Comment 3

[Boris Zbarsky [:bzbarsky]]

Bug 1032221 has a more reduced test case we should check in.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8447492 [details] [diff] [review]
trace-sources.patch

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

Please include the testcase from Bug 1032221.

::: js/src/vm/SavedStacks.cpp
@@ +439,5 @@
>      if (savedFrameProto && IsObjectAboutToBeFinalized(&savedFrameProto)) {
>          savedFrameProto = nullptr;
>      }
>  }
> +/* static */ void

Missing newline above.

@@ +444,5 @@
> +SavedStacks::trace(JSTracer *trc)
> +{
> +    if (!pcLocationMap.initialized()) {
> +        return;
> +    }

No braces around 1-line blocks.

@@ +453,5 @@
> +        MarkString(trc, &loc.source, "SavedStacks::PCLocationMap's memoized script source name");
> +    }
> +}
> +
> +

Extra newline here.

Comment 5

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

Attachment: trace-sources.patch

Comment 6

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

Comment on attachment 8448311 [details] [diff] [review]
trace-sources.patch

Carrying over r+

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er... you pulled in my testing patch, it looks like.

Comment 8

[Boris Zbarsky [:bzbarsky]]

But not the testcase?

Comment 9

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

Attachment: trace-sources.patch

Adds AutoLocationValueRooter and MutableHandleLocationValue.

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

Comment 10

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

Hazard analysis passes now!

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8448998 [details] [diff] [review]
trace-sources.patch

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

Great! r=me

::: js/src/vm/SavedStacks.h
@@ +174,5 @@
> +      public:
> +        inline MOZ_IMPLICIT MutableHandleLocationValue(AutoLocationValueRooter *location)
> +            : location(location) {}
> +
> +        MutableHandleLocationValue &operator=(LocationValue &loc) {

Please call this .set() to match the other MutableHandles: it's not clear using operator= whether you are setting the pointed to thing or the mutable handle itself.

Comment 12

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

Attachment: trace-sources.patch

s/=/set/

Comment 13

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab90208b363

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ab90208b363