Closed Bug 972591 Opened 6 years ago Closed 6 years ago

Convert XBL from using nsObjectHashtable to nsClassHashtable

Categories

(Core :: XBL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(4 files)

No description provided.
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.
Attachment #8376893 - Flags: review?(nfroyd)
Attachment #8376893 - Flags: review?(bzbarsky)
Likewise.
Attachment #8376894 - Flags: review?(nfroyd)
Attachment #8376894 - Flags: review?(bzbarsky)
No hurry on these reviews, I'm just slowly chipping away at these old hashtables.
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?
Attachment #8376893 - Flags: review?(nfroyd) → review+
Attachment #8376894 - Flags: review?(nfroyd) → review+
> 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 on attachment 8376892 [details] [diff] [review]
part 1 - Remove some trailing whitespace from nsXBLPrototypeBinding.

r=me
Attachment #8376892 - Flags: review?(bzbarsky) → review+
Comment on attachment 8376893 [details] [diff] [review]
part 2 - Convert nsXBLPrototypeBinding::mAttributeTable to nsClassHashtable.

r=me
Attachment #8376893 - Flags: review?(bzbarsky) → review+
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
Attachment #8376894 - Flags: review?(bzbarsky) → review+
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
Whiteboard: [leave open]
Bah, that didn't fix the leaks.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/0f213da04137
https://hg.mozilla.org/mozilla-central/rev/df1edcb49a35
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 976072
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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);
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.
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.
Attachment #8381524 - Flags: review?(bzbarsky)
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 on attachment 8381524 [details] [diff] [review]
part 3b - Don't destroy the binding when we remove it from the table.

r=me
Attachment #8381524 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/9d212681ddb7
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.