1055206 - Even more SavedStacks crashes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

https://tbpl.mozilla.org/php/getParsedLog.php?id=46184815&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=46184904&tree=Fx-Team

Like:

11:57:40  WARNING -  PROCESS-CRASH | chrome://mochitests/content/chrome/toolkit/devtools/server/tests/mochitest/test_memory_allocations_01.html | application crashed [@ js::SavedStacksMetadataCallback(JSContext*, JSObject**)]
11:57:40     INFO -  Crash dump filename: /tmp/tmp378ZVu.mozrunner/minidumps/6edb41ce-ff8c-24af-0f0f094b-5f05ca57.dmp
11:57:40     INFO -  Operating system: Linux
11:57:40     INFO -                    0.0.0 Linux 3.2.0-23-generic-pae #36-Ubuntu SMP Tue Apr 10 22:19:09 UTC 2012 i686
11:57:40     INFO -  CPU: x86
11:57:40     INFO -       GenuineIntel family 6 model 62 stepping 4
11:57:40     INFO -       1 CPU
11:57:40     INFO -  Crash reason:  SIGSEGV
11:57:40     INFO -  Crash address: 0x0
11:57:40     INFO -  Thread 0 (crashed)
11:57:40     INFO -   0  libxul.so!js::SavedStacksMetadataCallback(JSContext*, JSObject**) [Shape.h:085acacbd031 : 898 + 0x0]
11:57:40     INFO -      eip = 0xb54254c8   esp = 0xbfce94d0   ebp = 0xbfce94f8   ebx = 0xb6e5cd6c
11:57:40     INFO -      esi = 0xb721dc20   edi = 0xbfce94ec   eax = 0x00000000   ecx = 0xbfce94e4
11:57:40     INFO -      edx = 0xbfce955c   efl = 0x00010202
11:57:40     INFO -      Found by: given as instruction pointer in context
11:57:40     INFO -   1  libxul.so!js::NewDenseAllocatedArray(js::ExclusiveContext*, unsigned int, JSObject*, js::NewObjectKind) [jscompartment.h:085acacbd031 : 364 + 0x8]
11:57:40     INFO -      eip = 0xb5024655   esp = 0xbfce9500   ebp = 0xbfce95b8   ebx = 0xb6e5cd6c
11:57:40     INFO -      esi = 0xbfce9590   edi = 0xb721dc2c
11:57:40     INFO -      Found by: call frame info
11:57:40     INFO -   2  libxul.so!JS_NewArrayObject(JSContext*, unsigned int) [jsapi.cpp:085acacbd031 : 3763 + 0x15]
11:57:40     INFO -      eip = 0xb52e19fe   esp = 0xbfce95c0   ebp = 0xbfce95d8   ebx = 0xb6e5cd6c
11:57:40     INFO -      esi = 0xb721dc20   edi = 0xbfce9810
11:57:40     INFO -      Found by: call frame info
11:57:40     INFO -   3  libxul.so!mozilla::dom::MutationCallback::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Sequence<mozilla::dom::OwningNonNull<nsDOMMutationRecord> > const&, nsDOMMutationObserver&, mozilla::ErrorResult&) [MutationObserverBinding.cpp:085acacbd031 : 529 + 0x4]
11:57:40     INFO -      eip = 0xb417e0e3   esp = 0xbfce95e0   ebp = 0xbfce96a8   ebx = 0xb6e5cd6c
11:57:40     INFO -      esi = 0xb721dc20   edi = 0xbfce9810
11:57:40     INFO -      Found by: call frame info
11:57:40     INFO -   4  libxul.so!void mozilla::dom::MutationCallback::Call<nsDOMMutationObserver*>(nsDOMMutationObserver* const&, mozilla::dom::Sequence<mozilla::dom::OwningNonNull<nsDOMMutationRecord> > const&, nsDOMMutationObserver&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) [MutationObserverBinding.h:085acacbd031 : 180 + 0x19]
11:57:40     INFO -      eip = 0xb459eb7f   esp = 0xbfce96b0   ebp = 0xbfce97c8   ebx = 0xb6e5cd6c
11:57:40     INFO -      esi = 0xb721dc20   edi = 0x9b05b180
11:57:40     INFO -      Found by: call frame info
11:57:40     INFO -   5  libxul.so!nsDOMMutationObserver::HandleMutation() [nsDOMMutationObserver.cpp:085acacbd031 : 617 + 0x13]
11:57:40     INFO -      eip = 0xb459ed40   esp = 0xbfce97d0   ebp = 0xbfce9838   ebx = 0xb6e5cd6c
11:57:40     INFO -      esi = 0x7a6c5f00   edi = 0xbfce9804
11:57:40     INFO -      Found by: call frame info
11:57:40     INFO -   6  libxul.so!nsDOMMutationObserver::HandleMutationsInternal() [nsDOMMutationObserver.cpp:085acacbd031 : 657 + 0xd]
11:57:40     INFO -      eip = 0xb459eea1   esp = 0xbfce9840   ebp = 0xbfce9888   ebx = 0xb6e5cd6c
11:57:40     INFO -      esi = 0x00000000   edi = 0xb6ee1910
11:57:40     INFO -      Found by: call frame info
11:57:40     INFO -   7  libxul.so!nsContentUtils::LeaveMicroTask() [nsDOMMutationObserver.h:085acacbd031 : 412 + 0x4]

Comment 1

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

Attachment: null-frame-on-log-allocation-site.patch

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

Comment 2

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

Attachment: null-frame-on-log-allocation-site.patch

This just fixes the bad header include ordering. Looked like everything else was fine in that last try push.

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

Comment 3

[Jim Blandy :jimb]

Apparently there's a Simpsons bit where there's an inspector at a bottle factory who gets distracted and lets a bottle go by with a human head in it. I looked at the patch that introduced the bug... r=jimb

Comment 4

[Jim Blandy :jimb]

Comment on attachment 8475259 [details] [diff] [review]
null-frame-on-log-allocation-site.patch

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

I think there's a small change needed here; please revise, and I'll review promptly.

::: js/src/vm/Debugger.h
@@ +787,5 @@
>  bool
>  Debugger::onLogAllocationSite(JSContext *cx, HandleSavedFrame frame)
>  {
> +    if (!frame)
> +        return true;

I think this isn't correct: we need to track allocation that occurs with no JS frames on the stack, as well. We should (soon) be able to associate reasons with such allocation, so even without JS frames it won't be entirely mysterious.

We should push null on the allocation log when such allocations occur. I think this should be a quick patch:

- change JS_ASSERT(...) in the AllocationSite constructor to JS_ASSERT_IF(frame, ...)

- change ObjectValue in drainAllocationsLog to ObjectOrNullValue.

Unfortunately, I think this will be impossible to test in the shell, because it never does allocations with no JS frames on the stack.

Comment 5

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

Attachment: null-frame-on-log-allocation-site.patch

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

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8475427 [details] [diff] [review]
null-frame-on-log-allocation-site.patch

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

\o/

Comment 7

[Jim Blandy :jimb]

Attachment: Document meaning of `null` entries in drainAllocationsLog result.

This should be landed along with the code.

Comment 8

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

Attachment: null-frame-on-log-allocation-site.patch

Adds the missing JS:: that caused the 2 failures on try.

Comment 9

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

Comment on attachment 8475432 [details] [diff] [review]
Document meaning of `null` entries in drainAllocationsLog result.

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

Thanks.

Comment 10

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35c265c62076
https://hg.mozilla.org/integration/mozilla-inbound/rev/c181933a8273

Comment 11

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

Attachment: only-mark-non-null-allocation-site-frame.patch

Quick follow up to only mark |AllocationSite::frame|s that aren't null.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 8475672 [details] [diff] [review]
only-mark-non-null-allocation-site-frame.patch

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

I should have caught that. Thanks.

Comment 13

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79ce51162e77

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/35c265c62076
https://hg.mozilla.org/mozilla-central/rev/c181933a8273
https://hg.mozilla.org/mozilla-central/rev/79ce51162e77