1476141 - JS:ubi::Nodes representing DOM structure need to display in more detail

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[Kris Wright :KrisWright]

Nodes that report from DOM structure in Bug 1474383 only show as "other" category under the concrete type name "nsINode". We want to add a new coarse type for DOM nodes and traverse it in the Census in such a way that we can get more detailed node names from nsINode. nsINode already has a NodeName() function that returns a char16_t* pointer, so we need to make a non-owning function that we can use to get this name and display it for DOM node coarsetypes.

Comment 2

[Kris Wright :KrisWright]

Attachment: JS::ubi::Nodes represent DOM structure in more detail

Added a new CoarseType that refers to DOM nodes. Updated the trees to represent the CoarseType. Created a new type of count in the heap snapshot that sorts the data by a more descriptive type name. Created the descriptive in JS::ubi::Base.

Comment 3

[Kris Wright :KrisWright]

Attachment: JS::ubi::Nodes represent DOM structure in more detail

Added a new CoarseType that refers to DOM nodes. Updated the trees to represent the CoarseType. Created a new type of count in the heap snapshot that sorts the data by a more descriptive type name. Created the descriptive in JS::ubi::Base.

Comment 4

[Kris Wright :KrisWright]

Attachment: JS::ubi::Nodes represent DOM structure in more detail

Added a new CoarseType that refers to DOM nodes. Updated the trees to represent the CoarseType. Created a new type of count in the heap snapshot that sorts the data by a more descriptive type name. Created the descriptive in JS::ubi::Base.

Comment 5

[Kris Wright :KrisWright]

Attachment: updated test files to handle a new coarsetype

Some test files were affected by adding a new CoarseType, particularly in XPCShell tests. New rules were added to handle this CoarseType, and in a few cases the IDs of deserialized nodes had to be adjusted. The colors of CoarseTypes have also changed slightly and the resulting tests had to be updated.

Comment 6

[Kris Wright :KrisWright]

Attachment: Part 2: updated test files to handle a new coarsetype

Some test files were affected by adding a new CoarseType, particularly in XPCShell tests. New rules were added to handle this CoarseType, and in a few cases the IDs of deserialized nodes had to be adjusted. The colors of CoarseTypes have also changed slightly and the resulting tests had to be updated.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 8993116 [details] [diff] [review]
JS::ubi::Nodes represent DOM structure in more detail

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

This is great stuff. I've asked for two non-trivial changes to the code.

The comments in js/public/UbiNodeCensus.h need updating, I think.

The comment on the ByCoarseType type needs to be updated.

The documentation for takeCensus needs to be updated to mention the new behavior of the 'coarseType' breakdown:
https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/js/src/doc/Debugger/Debugger.Memory.md#278-304
https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/js/src/doc/Debugger/Debugger.Memory.md#373-402

::: devtools/shared/heapsnapshot/HeapSnapshot.cpp
@@ +284,5 @@
>        return false;
>    }
>  
> +  const char16_t* descriptiveTypeName = nullptr;
> +  if(node.descriptiveTypeNameOrRef_case() != protobuf::Node::DESCRIPTIVETYPENAMEORREF_NOT_SET) {

Space after `if`: `if (node.`...

::: js/public/UbiNode.h
@@ +628,5 @@
>  
> +    // Some JSObjects are reflectors to DOM objects.
> +    // Specialized DOM classes need to return their descriptive type names.
> +    // This type is non-owning and only exists in the scope of the object in Which
> +    // the data has membership.

Hmm. This doesn't mention that this function might return nullptr, or what that means, and the "non-owning" explanation isn't very clear. How about:

// In some cases, Concrete<T> can return a more descriptive
// referent type name than simply `T`. This method returns an
// identifier as specific as is efficiently available.
// The string returned is borrowed from the ubi::Node's referent.
// If nothing more specific than typeName() is available, return nullptr.

::: js/src/vm/UbiNodeCensus.cpp
@@ +347,4 @@
>  
>  // A hash map mapping from C strings to counts.
>  using CStringCountMap = HashMap<const char*, CountBasePtr, CStringHasher, SystemAllocPolicy>;
> +using C16StringCountMap = HashMap<const char16_t*, CountBasePtr, DefaultHasher<const char16_t*>,

I think DefaultHasher<const char16_t*> is just going to be PointerHasher:

https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/js/public/HashTable.h#625-629

I think you're going to need to make a copy of CStringHasher for char16_t strings:

https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/js/public/HashTable.h#680-690

Fortunately, it seems like mozilla::HashString already has a specialization for char16_t strings, so it's almost an exact copy, just with a different character type:

https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/mfbt/HashFunctions.h#274-278

C16StringCount is also going to need to own its keys; but I'll get to that below.

@@ +404,5 @@
>  }
>  
> +template <class Map, class GetName>
> +static PlainObject*
> +countMap16ToObject(JSContext* cx, Map& map, GetName getName) {

It's a pity we have to clone this function, just because of the character type. This is fine for now, but please file a follow-up bug to clean this up. Perhaps the GetName function could return a JSAtom directly.

@@ +555,5 @@
>  
> +class ByDomObjectClass : public CountType {
> +    // A table mapping class names to their counts. Mimics behavior of
> +    // ByUbiNodeType but behavior is altered for DOM object class names.
> +    // Objects missing their descriptive type name will use their concrete type name.

Nice comment.

@@ +616,5 @@
> +    MOZ_ASSERT(name);
> +    Table::AddPtr p = count.table.lookupForAdd(name);
> +    if (!p) {
> +        CountBasePtr classCount(classesType->makeCount());
> +        if (!classCount || !count.table.add(p, name, std::move(classCount)))

The contract of descriptiveTypeName says that the string it returns lives only as long as the ubi::Node's referent. But a census's tree of counts needs to be safe to retain even while we do things that could cause GC, like the 'report' methods allocating JS objects.

This code happens to work at the moment, because the only ubi::Nodes that override descriptiveTypeName (I think) are nsINode subclasses, which just borrow the nsNodeInfo's NodeName strings. Those (again, I think) are kept in a hash table that is owned by the HTML document:

https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/dom/base/nsNodeInfoManager.h#157

That almost certainly outlives the census, so we can't crash.

But JS::ubi::Node::descriptiveTypeName should accurately state its contract (in particular, the lifetime of the string it returns) and the census code should respect that contract. If we don't keep this sort of discipline, the code will begin to require close reading of modules that are not obviously related in order to avoid memory allocation mistakes. That's asking for trouble.

I think the fix is to make ByDomObjectClass::Table own its keys, much the way ByFilename::Table does. That hash table uses UniqueCString as its key type. You can crib from ByFilename for the types and methods.

Comment 8

[Kris Wright :KrisWright]

Attachment: JS::ubi::Nodes represent DOM structure in more detail

Added a new CoarseType that refers to DOM nodes. Updated the trees to represent the CoarseType. Created a new type of count in the heap snapshot that sorts the data by a more descriptive type name. Created the descriptive in JS::ubi::Base.

Comment 9

[Kris Wright :KrisWright]

Attachment: JS::ubi::Nodes represent DOM structure in more detail

Added a new CoarseType that refers to DOM nodes. Updated the trees to represent the CoarseType. Created a new type of count in the heap snapshot that sorts the data by a more descriptive type name. Created the descriptive in JS::ubi::Base.

Comment 10

[Kris Wright :KrisWright]

(In reply to Jim Blandy :jimb from comment #7)
> I think you're going to need to make a copy of CStringHasher for char16_t
> strings

Added the C16StringHasher, but I realized in the middle of it that the methods I could have used in Text.h to strcmp two char16_t strings wouldn't be available in this context (because I can't include util/Text.h in HashTable.h, unless there's a way around this that I don't know about other than depending on methods exclusively from UbiNodeCensus.cpp). In the meantime I wrote my own comparison for C16StringHasher::match as you'll see in the updated patch. I'm not fond of this solution when libraries do exist, though.

Comment 11

[Kris Wright :KrisWright]

Attachment: JS::ubi::Nodes represent DOM structure in more detail

Added a new CoarseType that refers to DOM nodes. Updated the trees to represent the CoarseType. Created a new type of count in the heap snapshot that sorts the data by a more descriptive type name. Created the descriptive in JS::ubi::Base.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 8995282 [details] [diff] [review]
JS::ubi::Nodes represent DOM structure in more detail

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

::: js/public/HashTable.h
@@ +21,4 @@
>  #include "mozilla/TypeTraits.h"
>  #include "mozilla/UniquePtr.h"
>  
> +#include <string>

This should come out.

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +387,4 @@
>  
>          Use the breakdown value <i>strings</i> for JavaScript strings.
>  
> +        Use the breakdown value <i>domNode</i> for items that come from DOM.

I would just say "... for DOM nodes."

::: js/src/vm/UbiNodeCensus.cpp
@@ +607,5 @@
> +ByDomObjectClass::makeCount()
> +{
> +    CountBasePtr classesCount(classesType->makeCount());
> +    if (!classesCount)
> +        return nullptr;

Bit of copy-pasta here: this is never used. We create sub-counts on demand in ByDomObjectClass::count.

Comment 13

[Kris Wright :KrisWright]

Attachment: Part 1: JS::ubi::Nodes represent DOM structure in more detail

Added a new CoarseType that refers to DOM nodes. Updated the trees to represent the CoarseType. Created a new type of count in the heap snapshot that sorts the data by a more descriptive type name. Created the descriptive in JS::ubi::Base.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdaeee8bac86
updated test files to handle a new coarsetype r=jlmb

Comment 15

[Dorel Luca [:dluca]]

Backed out changeset fdaeee8bac86 (bug 1476141) for XPCshell failure on devtools/shared/heapsnapshot/tests/unit/test_census_diff_03.js

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=190520392&repo=mozilla-inbound

TEST-START | devtools/shared/heapsnapshot/tests/unit/test_census_diff_03.js
[task 2018-07-27T13:25:26.219Z] 13:25:26  WARNING -  TEST-UNEXPECTED-FAIL | devtools/shared/heapsnapshot/tests/unit/test_census_diff_03.js | xpcshell return code: 0
[task 2018-07-27T13:25:26.222Z] 13:25:26     INFO -  TEST-INFO took 522ms
[task 2018-07-27T13:25:26.224Z] 13:25:26     INFO -  >>>>>>>

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fdaeee8bac869feca0e6ac0e382560ac39e2df26

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12eb093c59b116052c1a400d2b2f72bcd09d52b0

Comment 16

[Kris Wright :KrisWright]

Comment on attachment 8995303 [details] [diff] [review]
Part 1: JS::ubi::Nodes represent DOM structure in more detail

Carrying over the r+ from the last patch

Comment 17

[Kris Wright :KrisWright]

(In reply to Dorel Luca [:dluca] from comment #15)
> Backed out changeset fdaeee8bac86 (bug 1476141) for XPCshell failure on
> devtools/shared/heapsnapshot/tests/unit/test_census_diff_03.js
> 
> Log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=190520392&repo=mozilla-
> inbound
> 
> TEST-START | devtools/shared/heapsnapshot/tests/unit/test_census_diff_03.js
> [task 2018-07-27T13:25:26.219Z] 13:25:26  WARNING -  TEST-UNEXPECTED-FAIL |
> devtools/shared/heapsnapshot/tests/unit/test_census_diff_03.js | xpcshell
> return code: 0
> [task 2018-07-27T13:25:26.222Z] 13:25:26     INFO -  TEST-INFO took 522ms
> [task 2018-07-27T13:25:26.224Z] 13:25:26     INFO -  >>>>>>>
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=fdaeee8bac869feca0e6ac0e382560ac39e2df26
> 
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 12eb093c59b116052c1a400d2b2f72bcd09d52b0

Whew, both patches go together. Should have carried over my r+ and noted that, my bad.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/354b929d3bb1
JS::ubi::Nodes represent DOM structure in more detail r=KrisWright
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1270da843f4
updated test files to handle a new coarsetype r=jimb

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/354b929d3bb1
https://hg.mozilla.org/mozilla-central/rev/c1270da843f4