[webkit-reviews] review canceled: [Bug 172848] Web Inspector: Improve ES6 Class instances in Heap Snapshot instances view : [Attachment 311883] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 172848: Web Inspector: Improve ES6 Class instances in Heap Snapshot
instances view
https://bugs.webkit.org/show_bug.cgi?id=172848

Attachment 311883: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=311883&action=review




--- Comment #15 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 311883
  --> https://bugs.webkit.org/attachment.cgi?id=311883
[PATCH] Proposed Fix

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

>>> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:245
>>> +		 if (JSGlobalObject* globalObject =
object->structure(vm)->globalObject()) {
>> 
>> I don't think this is possible. All objects have a global
> 
> Hmm, I recall hitting a crash but I didn't debug what kind of object it was.
I'll get more information.

So it seems this can happen.

The instance I have in the debugger is a JSFinalObject with a valid, empty
structure, and the structure has no globalObject.

It appears to be:

    iterationTerminator.set(*this, JSFinalObject::create(*this,
JSFinalObject::createStructure(*this, 0, jsNull(), 1)));

Confirmed:

    Process 85739 resuming
    Process 85739 stopped
    * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS
(code=1, address=0xbbadbeef)
	frame #0: 0x0000000108638594 JavaScriptCore`::WTFCrash() at
Assertions.cpp:292
       289		globalHook();
       290	
       291	    WTFReportBacktrace();
    -> 292	    *(int *)(uintptr_t)0xbbadbeef = 0;
       293	    // More reliable, but doesn't say BBADBEEF.
       294	#if COMPILER(GCC_OR_CLANG)
       295	    __builtin_trap();

    (lldb) up
    frame #1: 0x0000000107353486
JavaScriptCore`JSC::JSObject::globalObject(this=0x000000010f1e80a0) const at
JSObject.h:782
       779	
       780	    JSGlobalObject* globalObject() const
       781	    {
    -> 782		ASSERT(structure()->globalObject());
       783		ASSERT(!isGlobalObject() ||
((JSObject*)structure()->globalObject()) == this);
       784		return structure()->globalObject();
       785	    }

    (lldb) p this
    (JSC::JSObject *) $10 = 0x000000010f1e80a0

    (lldb) p type()
    (JSC::JSType) $11 = FinalObjectType

    (lldb) p vm()->iterationTerminator.get() == this
    (bool) $12 = true

Given this information I think its safe to remove the check from
JSObject::calculatedClassName, but I do have to handle this case here when I'm
getting information about every single Object in the heap.

---

Maybe iterationTerminator can change to be something other than an object type,
which would be an alternate solution.

>> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:258
>>	    if (!node.cell->isString()) {
> 
> What about Symbol?

I would expect symbols to behave like other objects.

>> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:40
>> +// node flags
> 
> Is it worth stating which JSC file this corresponds to?

I think having the name be HeapSnapshot.js should be enough to track down the
corresponding HeapSnapshotBuilder.