Closed Bug 1294627 Opened 5 years ago Closed 5 years ago

Enhance gdb pretty printer for nsTHashtable to pierce alias templates and handle when it's used as a hashset, like for ManagedTemplate

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: asuth, Assigned: asuth)

Details

Attachments

(1 file)

While printing an mozilla:dom::ContentParent instance I noticed an error message like the following:
  AttributeError: 'NoneType' object has no attribute 'startswith'

This was due to the use of the "ManagedContainer" type within the generated PContentParent.h file:
  ManagedContainer<PAPZParent> mManagedPAPZParent;

ManagedContainer is an alias template defined like so:
  template<typename Protocol>
  using ManagedContainer = nsTHashtable<nsPtrHashKey<Protocol>>;

This was confusing the "walk_template_to_given_base" helper, so I added a strip_typedefs() there to fix that.

The other problem was that the pretty printer was assuming nsTHashtable<EntryType> would always be handed an nsBaseHashtableET<...> as its EntryType.  But ManagedContainer was just directly using a key type.  I believe this is the idiom I have heard talk of on IRC where nsTHashtable can be used like a set rather than a map.  Accordingly I've also taught it to recognize the difference and claim to be an array when it's a set.

Patch coming in a sec once I get the bug number.
The result is that the previously broken cases now look like:
    mManagedPSendStreamParent = mozilla::ManagedContainer, 
    mManagedPScreenManagerParent = mozilla::ManagedContainer = {0x7fc4962dcb40}, 
    mManagedPSmsParent = mozilla::ManagedContainer, 

Regrettably, however the type is being modeled by gdb, we're getting "mozilla::ManagedContainer" instead of "mozilla::ManagedContainer<PScreenManagerParent>".  According to "ptype", the field has type:
  mozilla::ManagedContainer mManagedPScreenManagerParent;
Although maybe if we dug around inside the Type hierarchy we could find it.

Normal maps still look like we expect:
  mControlledDocuments = nsRefPtrHashtable<nsISupportsHashKey, mozilla::dom::workers::ServiceWorkerRegistrationInfo>, 

  mRegisteringDocuments = nsClassHashtable<nsCStringHashKey, nsTArray<nsCOMPtr<nsIWeakReference> > >, 

  mRegistrationInfos = nsClassHashtable<nsCStringHashKey, mozilla::dom::workers::ServiceWorkerManager::RegistrationDataPerPrincipal> = {
    ["http://localhost:8000"] = [(mozilla::dom::workers::ServiceWorkerManager::RegistrationDataPerPrincipal *) 0x7fc4af1aee00],
    ["https://www.pokedex.org^userContextId=2"] = [(mozilla::dom::workers::ServiceWorkerManager::RegistrationDataPerPrincipal *) 0x7fc4af1ae980],
    ["https://googlechrome.github.io"] = [(mozilla::dom::workers::ServiceWorkerManager::RegistrationDataPerPrincipal *) 0x7fc4af1aea80]
  },
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Attachment #8780395 - Flags: review?(ttromey)
Comment on attachment 8780395 [details] [diff] [review]
Pierce alias templates, understand set usage. v1

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

Thank you for doing this.

I actually don't know the nsTHashtable code well enough to say whether this is correct.
However I think it's low risk to accept it.

I did find one nit in the to_string method, which I think should be fixed before landing.
r+ with that caveat.

::: python/gdbpp/gdbpp/thashtable.py
@@ +119,5 @@
>      def to_string(self):
> +        table = self.value['mTable']
> +        # Number of entries
> +        entryCount = table['mEntryCount']
> +

|table| and |entryCount| don't seem to be used here.
Attachment #8780395 - Flags: review?(ttromey) → review+
(In reply to Tom Tromey :tromey from comment #2)
> ::: python/gdbpp/gdbpp/thashtable.py
> @@ +119,5 @@
> >      def to_string(self):
> > +        table = self.value['mTable']
> > +        # Number of entries
> > +        entryCount = table['mEntryCount']
> > +
> 
> |table| and |entryCount| don't seem to be used here.

Whoops, thanks for catching!  I had temporarily added a " (%d entries)" suffix to the to_string() for debugging which used these, but I clearly was too quick in my patch cleanup.  I took it back out because the number of potentially empty buckets seemed noisy/confusing and knowing the number of occupied buckets requires traversing the buckets using the logic in children() which didn't actually seem all that valuable.  In the mManaged* list in ContentParent, for example, most of the sets had 0 entries or 1 empty entry, which seemed more distracting than useful when skimming the object.
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e002e37dff5b
Enhance gdb pretty printer for nsTHashtable to pierce alias templates and handle when it's used as a hashset, like for ManagedTemplate. r=tromey
https://hg.mozilla.org/mozilla-central/rev/e002e37dff5b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.