972591 - Convert XBL from using nsObjectHashtable to nsClassHashtable

Status: RESOLVED FIXED

(Core :: XBL, defect)

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove some trailing whitespace from nsXBLPrototypeBinding.

Trivial.

https://tbpl.mozilla.org/?tree=Try&rev=af50cdecffa3

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Convert nsXBLPrototypeBinding::mAttributeTable to nsClassHashtable.

The changes in this patch and the next one are really just hashtable changes, so bz probably doesn't really need to look at these, but I figure I should give him the right of refusal, as this is in the XBL component.  So feel free to just f+ it or something, Boris.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Convert nsXBLDocumentInfo::mBindingTable to nsClassHashtable.

Likewise.

Comment 4

[Andrew McCreight [:mccr8]]

No hurry on these reviews, I'm just slowly chipping away at these old hashtables.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8376893 [details] [diff] [review]
part 2 - Convert nsXBLPrototypeBinding::mAttributeTable to nsClassHashtable.

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

::: dom/xbl/nsXBLPrototypeBinding.cpp
@@ +318,2 @@
>                                          int32_t aNameSpaceID,
>                                          bool aRemoveFlag, 

Some missed trailing whitespace?

Comment 6

[Andrew McCreight [:mccr8]]

> Some missed trailing whitespace?
My general approach with removing trailing whitespace is to only do it on uninteresting lines, but maybe that's silly.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8376892 [details] [diff] [review]
part 1 - Remove some trailing whitespace from nsXBLPrototypeBinding.

r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8376893 [details] [diff] [review]
part 2 - Convert nsXBLPrototypeBinding::mAttributeTable to nsClassHashtable.

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8376894 [details] [diff] [review]
part 3 - Convert nsXBLDocumentInfo::mBindingTable to nsClassHashtable.

Do you still need the PromiseFlatCString bits here?  If not, please nix them.

r=me

Comment 10

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/25dc3246feca
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8048c03516b1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca2f958008b

I removed the PromiseFlatCString stuff.

Comment 11

[Andrew McCreight [:mccr8]]

Tons of valgrind failures in my push, and I think this is the most to blame, so I backed out part 2 and 3.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/95f63c571e35
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c0017c7020

Comment 12

[Andrew McCreight [:mccr8]]

Bah, that didn't fix the leaks.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25dc3246feca

Comment 14

[Andrew McCreight [:mccr8]]

Valgrind problem was another patch.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f213da04137
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df1edcb49a35

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f213da04137
https://hg.mozilla.org/mozilla-central/rev/df1edcb49a35

Comment 16

[Andrew McCreight [:mccr8]]

Backed out part 3 for causing bug 976072.
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b437275a4a8a

nsXBLDocumentInfo::RemovePrototypeBinding has the comment "This removes the binding without deleting it", and it works by calling Remove() on the hashtable.  With nsObjectHashtable, that does a remove without destroying the object, but with nsClassHashtable, the data is destroyed.

This actually makes sense for each, because the Remove of nsObjectHashtable returns the current object, so you can imagine that the responsibility for dealing with the object is passed along to the caller (for nsClassHashtable, it doesn't).  I didn't notice this here because RemovePrototypeBinding actually ignores the return value!  Oops.

Comment 17

[Andrew McCreight [:mccr8]]

This affects the Yahoo toolbar, because it hits some failure path on startup in nsXBLPrototypeBinding::Read:
    rv = mImplementation->Read(aStream, this);
    NS_ENSURE_SUCCESS(rv, rv);

Comment 18

[Andrew McCreight [:mccr8]]

Remove isn't called on the hashtable I removed in part 2, and that's the only other nsObjectHashtable conversion I've done that's stuck so far, so that should be okay.  The nsSupportsHashtable conversions use nsRefPtrHashtable, which has nsRefPtr for its data, instead of nsAutoPtr, so it should be okay.

Comment 19

[Andrew McCreight [:mccr8]]

merge of backout: https://hg.mozilla.org/mozilla-central/rev/b437275a4a8a

Comment 20

[Andrew McCreight [:mccr8]]

Attachment: part 3b - Don't destroy the binding when we remove it from the table.

This is wordier than the current implementation, but it is more explicit about the fact that the hash table is giving ownership of the object back to us, and we're intentionally dropping it on the floor.

I explored some alternative approaches that relied less on spooky-action-at-a-distance, where instead you passed around an nsAutoPtr<> that ended up set in the cases where you wanted to destroy something, and null otherwise, but it is pretty gross to pass around an nsAutoPtr to a method that is a reference to the object you are calling the method on, so I think the current setup is better.  I do have a patch to encapsulate it in nsXBLPrototypeBinding better, rather than living over in nsXBLDocumentInfo, so it is clearer that this weird interface isn't used in more than once place.

I'll land this merged in with part 3.

Comment 21

[Andrew McCreight [:mccr8]]

FWIW, I locally added a patch that checks for unused results of nsObjectHashtable::Remove, and this was the only one, so this problem shouldn't come up again with the other conversions.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8381524 [details] [diff] [review]
part 3b - Don't destroy the binding when we remove it from the table.

r=me

Comment 23

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d212681ddb7

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d212681ddb7