Add crash diagnostics for debugging unexpected zapped cells.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

...

[Radar WebKit Bug Importer]

<rdar://problem/53570112>

[Mark Lam]

Created attachment 374932 [details]
proposed patch.

Let's get some testing on the EWS first.  I'm still in the process of perf testing the patch locally.

[Mark Lam]

Created attachment 374938 [details]
proposed patch.

[Mark Lam]

Created attachment 374944 [details]
proposed patch.

[Michael Saboff]

Comment on attachment 374944 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=374944&action=review

r=me with some nits

> Source/JavaScriptCore/ChangeLog:10
> +        SlotVisitor::visitChildren().  If a zapped cell is detected, we we'll crash with

grammar *we well*.  May *we will* or *we'll*.

> Source/JavaScriptCore/heap/HeapCell.h:51
> +    // the rest of the cell header bits in tact for crash analysis uses.

*intact*

[Saam Barati]

Comment on attachment 374944 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=374944&action=review

> Source/JavaScriptCore/heap/HeapCell.h:55
> +        uint32_t* cellWords = reinterpret_cast_ptr<uint32_t*>(this);

bitwise_cast

> Source/JavaScriptCore/heap/HeapCell.h:60
> +    bool isZapped() const { return !*reinterpret_cast_ptr<const uint32_t*>(this); }

ditto

> Source/JavaScriptCore/heap/SlotVisitor.cpp:293
> +#if PLATFORM(MAC)

why not X86_64?

> Source/JavaScriptCore/heap/SlotVisitor.cpp:852
> +NEVER_INLINE void SlotVisitor::reportZappedCellAndCrash(JSCell* cell)
> +{
> +    MarkedBlock::Handle* foundBlock = nullptr;
> +    uint32_t* cellWords = reinterpret_cast_ptr<uint32_t*>(this);
> +
> +    uintptr_t cellAddress = bitwise_cast<uintptr_t>(cell);
> +    uintptr_t headerWord = cellWords[1];
> +    uintptr_t zapReason = cellWords[2];
> +    unsigned subspaceHash = 0;
> +    size_t cellSize = 0;
> +
> +    m_heap.objectSpace().forEachBlock([&] (MarkedBlock::Handle* block) {
> +        if (block->contains(cell)) {
> +            foundBlock = block;
> +            return IterationStatus::Done;
> +        }
> +        return IterationStatus::Continue;
> +    });
> +
> +    if (foundBlock) {
> +        subspaceHash = bmalloc::stringHash(foundBlock->subspace()->name());
> +        cellSize = foundBlock->cellSize();
> +    }
> +
> +    RELEASE_ASSERT(!cell->isZapped(), cellAddress, headerWord, zapReason, subspaceHash, cellSize);
> +}

adding all of this worries more than just crashing. Do we not think the crash alone is good enough? At least in the appendToMarkedStack case.

If you do keep it, please:
- use bitwise_cast
- Use WTF's string hash instead of bmalloc

[Saam Barati]

I didn't mean to clear Michael's r+, though I do have some hesitation. Last time we added a bunch of debugging code, we ended up in a worse place than just crashing.

[Saam Barati]

Comment on attachment 374944 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=374944&action=review

> Source/JavaScriptCore/heap/SlotVisitor.cpp:851
> +    RELEASE_ASSERT(!cell->isZapped(), cellAddress, headerWord, zapReason, subspaceHash, cellSize);

please make this CRASH_WITH_INFO instead. Just to avoid any chance someone raced and allocated over this.

[Mark Lam]

Created attachment 374997 [details]
proposed patch.

I've applied the feedback from Michael and Saam, and added some additional tooling to dump subspace hashes.

[Mark Lam]

Comment on attachment 374997 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=374997&action=review

> Source/JavaScriptCore/heap/SlotVisitor.cpp:849
> +#endif // PLATFORM(MAC)

I'll fix this comment before landing.  Should be CPU(X86_64).

[Yusuke Suzuki]

Comment on attachment 374997 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=374997&action=review

r=me with a nit.

> Source/JavaScriptCore/heap/SlotVisitor.cpp:834
> +    m_heap.objectSpace().forEachBlock([&] (MarkedBlock::Handle* block) {

This is potentially dangerous because m_blocks can be added by the main thread while the concurrent marker is working.
But maybe, it does not matter much since the crash trace itself offers much more profitable information already.
Can you add a comment about it with FIXME or invent something safer?

[Mark Lam]

Thanks for the reviews.  Preliminary perf testing results looks promising i.e. no significant change.  I'm going to go ahead and land this.  We can roll it out later if we find that it does regress performance.

[Mark Lam]

Landed in r247886: <http://trac.webkit.org/r247886>.

[Michael Catanzaro]

[975/2287] Building CXX object Source/...sources/UnifiedSource-ee8a7a7a-7.cpp.o
In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-ee8a7a7a-7.cpp:2:
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp: In member function ‘void JSC::SlotVisitor::reportZappedCellAndCrash(JSC::JSCell*)’:
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:828:15: warning: unused variable ‘cellAddress’ [-Wunused-variable]
  828 |     uintptr_t cellAddress = bitwise_cast<uintptr_t>(cell);
      |               ^~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:829:15: warning: unused variable ‘headerWord’ [-Wunused-variable]
  829 |     uintptr_t headerWord = cellWords[1];
      |               ^~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:830:15: warning: unused variable ‘zapReason’ [-Wunused-variable]
  830 |     uintptr_t zapReason = cellWords[2];
      |               ^~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:831:14: warning: variable ‘subspaceHash’ set but not used [-Wunused-but-set-variable]
  831 |     unsigned subspaceHash = 0;
      |              ^~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/SlotVisitor.cpp:832:12: warning: variable ‘cellSize’ set but not used [-Wunused-but-set-variable]
  832 |     size_t cellSize = 0;
      |            ^~~~~~~~

So this is kinda annoying to fix.

The obvious solution is to just use the UNUSED_VARIABLE() macro for each of these in SlotVisitor.cpp. But that's annoying because the variables are passed to CRASH_WITH_INFO(), so one would expect CRASH_WITH_INFO() to use them. It does for Clang and MSVCC, but not for the fallback GCC implementation. And writing a macro that passes the __VA_ARGS__ to UNUSED_VARIABLE() doesn't seem simple. But I think we could create a macro that calls a variadic function and just does nothing with its va_list.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 200214

[Mark Lam]

FYI, this patch has been rolled out in r247900: <https://trac.webkit.org/changeset/247900> because it regresses PLT5.

[Tadeu Zagallo]

(In reply to Michael Catanzaro from comment #14)
> The obvious solution is to just use the UNUSED_VARIABLE() macro for each of
> these in SlotVisitor.cpp. But that's annoying because the variables are
> passed to CRASH_WITH_INFO(), so one would expect CRASH_WITH_INFO() to use
> them. It does for Clang and MSVCC, but not for the fallback GCC
> implementation. And writing a macro that passes the __VA_ARGS__ to
> UNUSED_VARIABLE() doesn't seem simple. But I think we could create a macro
> that calls a variadic function and just does nothing with its va_list.

FWIW, there's `WTF_LAZY_FOR_EACH_TERM` in wtf/Forward.h. I believe you could use `WTF_LAZY_FOR_EACH_TERM(UNUSED_VARIABLE,  __VA_ARGS__)`

[Michael Catanzaro]

That seems good, thanks!

[Mark Lam]

I'll fix the CRASH_WITH_INFO unused parameters issue separately in https://bugs.webkit.org/show_bug.cgi?id=200243.

[Mark Lam]

Created attachment 375343 [details]
proposed patch.

[EWS Watchlist]

Attachment 375343 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/CPU.cpp:74:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/JavaScriptCore/assembler/CPU.cpp:84:  Use nullptr instead of NULL.  [readability/null] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 375343 [details]
proposed patch.

r=me

[Mark Lam]

Created attachment 375346 [details]
patch for landing.

[EWS Watchlist]

Comment on attachment 375346 [details]
patch for landing.

Attachment 375346 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12849056

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases

[Mark Lam]

(In reply to Build Bot from comment #24)
> Comment on attachment 375346 [details]
> patch for landing.
> 
> Attachment 375346 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12849056
> 
> New failing tests:
> mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-
> validate-phases

This is unrelated. I ran this test on my slowest MacBookAir, and it runs just fine.

[Mark Lam]

Landed in r248143: <http://trac.webkit.org/r248143>.

[Mark Lam]

+ Build fix in r248162: <http://trac.webkit.org/r248162>.