Bug 172848

<rdar://problem/25709212>

Created attachment 311802 [details] [PATCH] Proposed Fix

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.

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

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

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.

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?

Created attachment 311805 [details] [PATCH] Proposed Fix

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

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

Created attachment 311883 [details] [PATCH] Proposed Fix

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.

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?

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.

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.

Created attachment 311903 [details] [PATCH] Proposed Fix

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.

Created attachment 311904 [details] [PATCH] Proposed Fix

Created attachment 311905 [details] [PATCH] Proposed Fix Ahh. Updated ChangeLog and addressed the 1 style issue. Sorry for the churn.

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.

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?

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

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

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

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.

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

Comment on attachment 311905 [details] [PATCH] Proposed Fix r=me

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

Comment on attachment 311905 [details] [PATCH] Proposed Fix Clearing flags on attachment: 311905 Committed r217807: <http://trac.webkit.org/changeset/217807>

All reviewed patches have been landed. Closing bug.

(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

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

Rolled this out in: https://trac.webkit.org/changeset/217843/webkit It was causing a HeapSnapshot test to crash.

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

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.

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

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.

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

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.

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.

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.

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

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

Created attachment 362359 [details] [PATCH] Proposed Fix Addressed Devin's comments.

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.

Created attachment 362394 [details] [PATCH] Proposed Fix This one should be good!

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.

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.

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

Reverted r241784 for reason: Broke all OpenSource builds. Committed r241785: <https://trac.webkit.org/changeset/241785>

<https://trac.webkit.org/r241787> I'll be watching bots.