Bug 172848 – Web Inspector: Improve ES6 Class instances in Heap Snapshot instances view

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Summary:
Improve ES6 Class instances in Heap Snapshot instances view

Instances in the Heap Snapshot view that make use of ES6 or ES5 style classes should get their own category, not be clumped all inside of "Object".

For example:

    class Foo {};
    let foo1 = new Foo;
    let foo2 = new Foo;

Currently both of `foo1` and `foo2` will be listed in the "Object" category.

It would be nicer if they were listed in their own "Foo" category.

[Joseph Pecoraro]

<rdar://problem/25709212>

[Joseph Pecoraro]

Created attachment 311802 [details]
[PATCH] Proposed Fix

[Build Bot]

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


ERROR: Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:238:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 16 files


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

[Joseph Pecoraro]

Created attachment 311803 [details]
[IMAGE] New Categories when inspecting Web Inspector

[Joseph Pecoraro]

Created attachment 311804 [details]
[IMAGE] Ability to Distinguish Class Instance & Native Instance of the same name

[Joseph Pecoraro]

Comment on attachment 311802 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:436
> +    return constructorFunction->name(vm);

On second thought, I think this should be a calculatedDisplayName. We should use the best name they have provided us.

[Sam Weinig]

Comment on attachment 311802 [details]
[PATCH] Proposed Fix

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

Does this only work for ES6 classes, or will it work for any new'd object?

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:411
> +String HeapSnapshotBuilder::betterClassNameForObject(VM& vm, JSObject* object)
> +{

How does this compare to JSObject::calculatedClassName?

[Joseph Pecoraro]

Created attachment 311805 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

> > Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:411
> > +String HeapSnapshotBuilder::betterClassNameForObject(VM& vm, JSObject* object)
> > +{
> 
> How does this compare to JSObject::calculatedClassName?

Hmm, that does look very similar, including being careful not to run user code. I'll give that a shot.

[Joseph Pecoraro]

> Hmm, that does look very similar, including being careful not to run user
> code. I'll give that a shot.

Here is a case in particular that is different right now:

    function Foo() {};
    Foo.prototype = { constructor: Foo };
    var instance = new Foo;

In this case there are at least 3 cells:

    1. Foo - a Function
    2. Foo.prototype - an Object
    3. instance - a Foo instance

JSObject::calculatedClassName produces the name "Foo" for (2). I think we should treat (2) more like an "Object". The betterClassNameForObject solved this by not going down the __proto__.constructor.name path if the object itself has a Constructor property.

I will try the same thing here and see what else is affected. It looks like this is only used by the inspector for certain object previews and the type profiler.

[Joseph Pecoraro]

Created attachment 311883 [details]
[PATCH] Proposed Fix

[Build Bot]

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


ERROR: Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:244:  Declaration has space between type name and * in JSObject *object  [whitespace/declaration] [3]
Total errors found: 1 in 18 files


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

[Saam Barati]

Comment on attachment 311883 [details]
[PATCH] Proposed Fix

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

mostly LGTM. Just a few questions/suggestions

> Source/JavaScriptCore/ChangeLog:26
> +        as a JSObject without a GlobalObject, and an object which doesn't

This should not be possible. All objects have a global object.

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:23
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

also worth changing above to include 2017

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:238
> +        if (node.cell->isObject() && !strcmp(className, "Object")) {

I propose something like this:
`!strcmp(className, JSObject::info()->className)` that way it's not hard coded in two places. I also think it's easier to read that way.

> 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

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

What about Symbol?

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:261
> +                flags |= Internal;

I'm confused as to why this isn't mutually exclusive with ObjectSubtype. What cell type were you seeing that has both bits set?

> Source/JavaScriptCore/runtime/JSObject.cpp:530
> +    if (!globalObject)

ditto

> Source/JavaScriptCore/runtime/JSObject.cpp:551
> +                    JSValue constructorValue = slot.getValue(exec, constructor);
> +                    if (constructorValue.isCell()) {
> +                        if (JSCell* constructorCell = constructorValue.asCell()) {
> +                            if (JSObject* ctorObject = constructorCell->getObject()) {

I think these could be condensed into:
if (JSObject* ctorObject = jsDynamicCast<JSObject*>(slot.getValue(exec, constructor)) ...

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:40
> +// node flags

Is it worth stating which JSC file this corresponds to?

[Joseph Pecoraro]

Comment on attachment 311883 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:238
>> +        if (node.cell->isObject() && !strcmp(className, "Object")) {
> 
> I propose something like this:
> `!strcmp(className, JSObject::info()->className)` that way it's not hard coded in two places. I also think it's easier to read that way.

Nice!

>> 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.

>> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:261
>> +                flags |= Internal;
> 
> I'm confused as to why this isn't mutually exclusive with ObjectSubtype. What cell type were you seeing that has both bits set?

I guess figuring out the answering to that question above, should answer this. It just needs to be an Object that doesn't have a globalObject.

[Joseph Pecoraro]

Comment on attachment 311883 [details]
[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.

[Joseph Pecoraro]

Created attachment 311903 [details]
[PATCH] Proposed Fix

[Build Bot]

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


ERROR: Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:244:  Declaration has space between type name and * in JSObject *object  [whitespace/declaration] [3]
Total errors found: 1 in 18 files


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

[Joseph Pecoraro]

Created attachment 311904 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 311905 [details]
[PATCH] Proposed Fix

Ahh. Updated ChangeLog and addressed the 1 style issue. Sorry for the churn.

[Saam Barati]

This breaks an invariant I’ve been programming against where I’ve thought “Object -> !!globalObject”

Perhaps I’m wrong. CCing Geoff, Fil, Yusuke, Keith, Mark, for their thoughts.

[Saam Barati]

Comment on attachment 311883 [details]
[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.

interestingly, "iterationTerminator" does not even look like it's used. My suggestion would be to remove it.
I would also be in favor of adding an assertion in Structure constructor if the type is >= objectType, that globalObject is non-null.

Joe, want me to do this as a blocking bug to this?

[Filip Pizlo]

(In reply to Saam Barati from comment #21)
> Comment on attachment 311883 [details]
> [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.
> 
> interestingly, "iterationTerminator" does not even look like it's used. My
> suggestion would be to remove it.
> I would also be in favor of adding an assertion in Structure constructor if
> the type is >= objectType, that globalObject is non-null.
> 
> Joe, want me to do this as a blocking bug to this?

How do proxies reason about their global object?  In particular JSProxy.

[Joseph Pecoraro]

> > Maybe iterationTerminator can change to be something other than an object type, which would be an alternate solution.
> 
> interestingly, "iterationTerminator" does not even look like it's used. My
> suggestion would be to remove it.
> I would also be in favor of adding an assertion in Structure constructor if
> the type is >= objectType, that globalObject is non-null.
> 
> Joe, want me to do this as a blocking bug to this?

I'll attempt it. I noticed some other unused members of VM as well.

[Saam Barati]

(In reply to Filip Pizlo from comment #22)
> (In reply to Saam Barati from comment #21)
> > Comment on attachment 311883 [details]
> > [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.
> > 
> > interestingly, "iterationTerminator" does not even look like it's used. My
> > suggestion would be to remove it.
> > I would also be in favor of adding an assertion in Structure constructor if
> > the type is >= objectType, that globalObject is non-null.
> > 
> > Joe, want me to do this as a blocking bug to this?
> 
> How do proxies reason about their global object?  In particular JSProxy.
I'm not sure in general, but JSProxy's structure is indeed created with a non-null global object.

[Joseph Pecoraro]

Arg, I lost my comment.

In any case I filed:
<https://webkit.org/b/172939> Maintain an Invariant that a JSObject always has a GlobalObject

It seems to me it can be done either before or after this as a follow-up. I'll leave that to you guys.

[Saam Barati]

(In reply to Joseph Pecoraro from comment #25)
> Arg, I lost my comment.
> 
> In any case I filed:
> <https://webkit.org/b/172939> Maintain an Invariant that a JSObject always
> has a GlobalObject
> 
> It seems to me it can be done either before or after this as a follow-up.
> I'll leave that to you guys.

Sounds like a follow-up to me.

[Saam Barati]

Comment on attachment 311905 [details]
[PATCH] Proposed Fix

r=me

[Joseph Pecoraro]

Devin also briefly reviewed the inspector frontend portions and gave me a LGTM.

[WebKit Commit Bot]

Comment on attachment 311905 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 311905

Committed r217807: <http://trac.webkit.org/changeset/217807>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #29)
> Comment on attachment 311905 [details]
> [PATCH] Proposed Fix
> 
> Clearing flags on attachment: 311905
> 
> Committed r217807: <http://trac.webkit.org/changeset/217807>

inspector/model/remote-object.html is failing after this change:

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/r217819%20(2187)/results.html

[Joseph Pecoraro]

This might be okay. I'll look tomorrow. If you need to you may skip the test

[Joseph Pecoraro]

Rolled this out in:
https://trac.webkit.org/changeset/217843/webkit

It was causing a HeapSnapshot test to crash.

[Ryan Haddad]

*** Bug 173014 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

Comment on attachment 311905 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:249
> +                    className = JSObject::calculatedClassName(object).utf8().data();

I'm guessing we can't just do this because the WTF::String's data we are grabbing here, might be a temporary and go away. So we'll need a safer way to use the string data.

The classNameIndexes set currently uses a `const char *` but if we convert that to a WTF::String that should be enough.

[Joseph Pecoraro]

Created attachment 362211 [details]
[PATCH] Proposed Fix

After a long long wait, I've gotten back to this again!

• This drive-by fixes JSTexts/heapProfiler.
• Slight cleanup of the old patch
• Fixes the bad `const char*` use by switching to a Set of WTF::Strings

[Joseph Pecoraro]

Comment on attachment 362211 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/console/queryObjects-expected.txt:31
> -[ClassB, ClassC, ClassC]
> +[ClassB, ClassB, ClassC]

I'm not exactly sure what causes these... the changes to JSC::JSObject::calculatedClassName must have, but it doesn't modify anything in the UI. The UI still appears as I would expect.

[Devin Rousso]

Comment on attachment 362211 [details]
[PATCH] Proposed Fix

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

> JSTests/heapProfiler/class-names.js:25
> +    nodes = snapshot.nodesWithClassName("MyES5ClassDisplayName");

Should we assert/test that any of these specialized classes don't appear in the generic "object" category?

> LayoutTests/inspector/model/remote-object-expected.txt:3733
> -    "_description": "B"
> +    "_description": "A"

This doesn't sound right.  The prototype of `Beta` should be `B`.

> Source/JavaScriptCore/ChangeLog:25
> +        Handle some possible edge cases that were not handled before. Such

Grammar: "before, such as "

> Source/JavaScriptCore/ChangeLog:26
> +        as a JSObject without a GlobalObject, and an object which doesn't

Grammar: "GlobalObject, or an object"

> Source/JavaScriptCore/ChangeLog:27
> +        have a default getPrototype. Try to make the code a little clearer.

Is this case tested anywhere?  The code itself seems to early-return in this case, so would that mean it tries to go through the `methodTable` instead?

I think these changes deserve a more "in-depth" comment.

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:400
> +            if (JSGlobalObject* globalObject = object->structure(vm)->globalObject()) {

Isn't this the same as `object->globalObject(vm)`?

> Source/JavaScriptCore/runtime/JSObject.cpp:533
> +    // Get the display name of obj.__proto__.constructor.

Whereas the previous code simply retrieved `obj.constructor` directly?

> Source/JavaScriptCore/runtime/JSObject.cpp:534
> +    MethodTable::GetPrototypeFunctionPtr defaultGetPrototype = JSObject::getPrototype;

Is pulling this out into a variable really necessary?  It's only used once, and I feel like inlining `JSObject::getPrototype` is clear enough (or at the very least could be explained by a comment).

> Source/JavaScriptCore/runtime/JSObject.cpp:541
> +            if (protoObject->getPropertySlot(exec, vm.propertyNames->constructor, slot)) {

Should we add a `EXCEPTION_ASSERT(!scope.exception())`?

> Source/WebInspectorUI/ChangeLog:23
> +        Add a new Node isObjectType property based on the new data.

So in reality, this has been added purely (as of now) to make sure we use the right icon?  Not complaining/criticizing, just trying to understand.

> Source/WebInspectorUI/ChangeLog:30
> +        If a class contains 50% or more object type instances then it as such
> +        instead of defaulting to native.

Grammar: this sentence isn't right.

    If a class contains 50% of more object type instances, then treat it as such instead of default to "native".

> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotNodeProxy.js:37
> +        this.isObjectType = isObjectType;

Calling it `isObjectType` seems a bit "vague".  I think calling it `isCustomType` (custom values are always objects) or even the previously used `isObjectSubtype` (that way the code matches everywhere and can easily be searched) may be clearer.

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:60
> +//   - In Version 2, this became a bitmask so multiple flags could be included without modifying the size.

Clever!  This will also preserve compatibility because `(true && (1 << 0)) === true` whereas `(true && (1 << 1)) === false`.

[Joseph Pecoraro]

Comment on attachment 362211 [details]
[PATCH] Proposed Fix

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

>> JSTests/heapProfiler/class-names.js:25
>> +    nodes = snapshot.nodesWithClassName("MyES5ClassDisplayName");
> 
> Should we assert/test that any of these specialized classes don't appear in the generic "object" category?

Hmm, we can. Sure.

>> LayoutTests/inspector/model/remote-object-expected.txt:3733
>> +    "_description": "A"
> 
> This doesn't sound right.  The prototype of `Beta` should be `B`.

Yeah this is what I was saying before. Our description of the Class instance itself seems to look at the parent instead of itself. That said, our UI in Web Inspector still shows the right thing.

>> Source/JavaScriptCore/ChangeLog:27
>> +        have a default getPrototype. Try to make the code a little clearer.
> 
> Is this case tested anywhere?  The code itself seems to early-return in this case, so would that mean it tries to go through the `methodTable` instead?
> 
> I think these changes deserve a more "in-depth" comment.

The non-default getPrototype? That isn't tested anywhere. I don't recall how to test it (maybe Proxies?) but in general we only want to do prototype lookups internally if we can avoid running user code.

That said, the code doesn't early-return in that case, it should fall back to the normal JS subclass name path:

    object->methodTable(vm)->className(object, vm);
    object->classInfo(vm)->className
    "Object"_s

>> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:400
>> +            if (JSGlobalObject* globalObject = object->structure(vm)->globalObject()) {
> 
> Isn't this the same as `object->globalObject(vm)`?

Yes!

>> Source/JavaScriptCore/runtime/JSObject.cpp:533
>> +    // Get the display name of obj.__proto__.constructor.
> 
> Whereas the previous code simply retrieved `obj.constructor` directly?

I don't recall the importance of checking the defaultGetPrototype, but if there is a non-default getPrototype then it seems we would potentially be running user code or following a separate path than getting the direct prototype property. This is following a pattern in JavaScriptCore seen in other places.

>> Source/JavaScriptCore/runtime/JSObject.cpp:534
>> +    MethodTable::GetPrototypeFunctionPtr defaultGetPrototype = JSObject::getPrototype;
> 
> Is pulling this out into a variable really necessary?  It's only used once, and I feel like inlining `JSObject::getPrototype` is clear enough (or at the very least could be explained by a comment).

This just matches a common pattern in JavaScriptCore.

>> Source/JavaScriptCore/runtime/JSObject.cpp:541
>> +            if (protoObject->getPropertySlot(exec, vm.propertyNames->constructor, slot)) {
> 
> Should we add a `EXCEPTION_ASSERT(!scope.exception())`?

Sounds good. Seems I can use: `scope.assertNoException();`

>> Source/WebInspectorUI/ChangeLog:23
>> +        Add a new Node isObjectType property based on the new data.
> 
> So in reality, this has been added purely (as of now) to make sure we use the right icon?  Not complaining/criticizing, just trying to understand.

Yes

>> Source/WebInspectorUI/ChangeLog:30
>> +        instead of defaulting to native.
> 
> Grammar: this sentence isn't right.
> 
>     If a class contains 50% of more object type instances, then treat it as such instead of default to "native".

Hahah, yes thanks

>> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotNodeProxy.js:37
>> +        this.isObjectType = isObjectType;
> 
> Calling it `isObjectType` seems a bit "vague".  I think calling it `isCustomType` (custom values are always objects) or even the previously used `isObjectSubtype` (that way the code matches everywhere and can easily be searched) may be clearer.

I like isObjectSubtype.

>> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:60
>> +//   - In Version 2, this became a bitmask so multiple flags could be included without modifying the size.
> 
> Clever!  This will also preserve compatibility because `(true && (1 << 0)) === true` whereas `(true && (1 << 1)) === false`.

Previously this was sent as an explicit `0` or `1`, so yep.

[Joseph Pecoraro]

Comment on attachment 362211 [details]
[PATCH] Proposed Fix

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

>>> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotNodeProxy.js:37
>>> +        this.isObjectType = isObjectType;
>> 
>> Calling it `isObjectType` seems a bit "vague".  I think calling it `isCustomType` (custom values are always objects) or even the previously used `isObjectSubtype` (that way the code matches everywhere and can easily be searched) may be clearer.
> 
> I like isObjectSubtype.

Re-thinking this, I think "isObjectType" and "isObjectSubtype" are the same here. So I think I'm good keeping this as is. `isCustomType` or `isUserType` also seems unnecessary strong. These objects would be `Object`, but we are providing ab better className.

[Joseph Pecoraro]

Comment on attachment 362211 [details]
[PATCH] Proposed Fix

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

>>> JSTests/heapProfiler/class-names.js:25
>>> +    nodes = snapshot.nodesWithClassName("MyES5ClassDisplayName");
>> 
>> Should we assert/test that any of these specialized classes don't appear in the generic "object" category?
> 
> Hmm, we can. Sure.

All "Object" sub types, including Object will have this flag set. It is an object instance, and its name may be Object or made better.

[Joseph Pecoraro]

Comment on attachment 362211 [details]
[PATCH] Proposed Fix

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

>>> Source/JavaScriptCore/runtime/JSObject.cpp:541
>>> +            if (protoObject->getPropertySlot(exec, vm.propertyNames->constructor, slot)) {
>> 
>> Should we add a `EXCEPTION_ASSERT(!scope.exception())`?
> 
> Sounds good. Seems I can use: `scope.assertNoException();`

A similar assertion already exists below. Interesting that it potentially allows an exception

[Joseph Pecoraro]

Comment on attachment 362211 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/console/queryObjects-expected.txt:31
>> +[ClassB, ClassB, ClassC]
> 
> I'm not exactly sure what causes these... the changes to JSC::JSObject::calculatedClassName must have, but it doesn't modify anything in the UI. The UI still appears as I would expect.

Ahh, okay. So previously we were checking `

>>> LayoutTests/inspector/model/remote-object-expected.txt:3733
>>> +    "_description": "A"
>> 
>> This doesn't sound right.  The prototype of `Beta` should be `B`.
> 
> Yeah this is what I was saying before. Our description of the Class instance itself seems to look at the parent instead of itself. That said, our UI in Web Inspector still shows the right thing.

Okay, so we do have to check for an immediately `constructor` property before looking up to `prototype.constructor` for this case:

    Beta = class Beta {}; Beta.prototype

[Joseph Pecoraro]

Created attachment 362359 [details]
[PATCH] Proposed Fix

Addressed Devin's comments.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:556:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 20 files


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

[Joseph Pecoraro]

Comment on attachment 362359 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/runtime/JSObject.cpp:556
> +        if (LIKELY(structure->classInfo()->methodTable.getPrototype == defaultGetPrototype)) {        

Trailing whitespace is what the style bot is complaining about.

[Joseph Pecoraro]

Created attachment 362394 [details]
[PATCH] Proposed Fix

This one should be good!

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:556:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 20 files


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

[Mark Lam]

Comment on attachment 362394 [details]
[PATCH] Proposed Fix

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

r=me with issues resolved.

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:262
> +typedef enum {
> +    Internal      = 1 << 0,
> +    ObjectSubtype = 1 << 1,
> +} NodeFlags;

Why not use an enum class?  With unified sources, it's conceivable that an enum named "Internal" can easily conflict with another (if not presently, then perhaps in the future).

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:417
> +            if (!structure || !structure->globalObject())

I think the only time structure can be null is if the cell has been GCed.  Are you seeing this in the wild? m It's OK to leave this null check for now, but I would be surprised if this is actually needed.

> Source/JavaScriptCore/runtime/JSObject.cpp:528
> +    auto structure = object->structure();

It's clearer to use auto* here.

> Source/JavaScriptCore/runtime/JSObject.cpp:529
> +    auto globalObject = structure->globalObject();

Ditto.

> Source/JavaScriptCore/runtime/JSObject.cpp:532
> +    auto exec = globalObject->globalExec();

Ditto.

> Source/JavaScriptCore/runtime/JSObject.cpp:538
>          if (slot.isValue()) {

I think you need an "EXCEPTION_ASSERT(!scope.exception());" above this line to placate the exception check verifier.

> Source/JavaScriptCore/runtime/JSObject.cpp:562
> +                if (protoObject->getPropertySlot(exec, vm.propertyNames->constructor, slot)) {
> +                    if (slot.isValue()) {

Ditto: you'll need an "EXCEPTION_ASSERT(!scope.exception());" between these lines to placate the exception check verifier.

[Joseph Pecoraro]

<https://trac.webkit.org/r241784>

[Truitt Savell]

Reverted r241784 for reason:

Broke all OpenSource builds.

Committed r241785: <https://trac.webkit.org/changeset/241785>

[Joseph Pecoraro]

<https://trac.webkit.org/r241787>

I'll be watching bots.