Convert XBL from using nsObjectHashtable to nsClassHashtable

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 8376892 [details] [diff] [review]
part 1 - Remove some trailing whitespace from nsXBLPrototypeBinding.

Trivial.

https://tbpl.mozilla.org/?tree=Try&rev=af50cdecffa3
Attachment #8376892 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

5 years ago
Created attachment 8376893 [details] [diff] [review]
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.
Attachment #8376893 - Flags: review?(nfroyd)
Attachment #8376893 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 8376894 [details] [diff] [review]
part 3 - Convert nsXBLDocumentInfo::mBindingTable to nsClassHashtable.

Likewise.
Attachment #8376894 - Flags: review?(nfroyd)
Attachment #8376894 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

5 years ago
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+
(Assignee)

Comment 6

5 years ago
> 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+
(Assignee)

Comment 11

5 years ago
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
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Comment 12

5 years ago
Bah, that didn't fix the leaks.
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/0f213da04137
https://hg.mozilla.org/mozilla-central/rev/df1edcb49a35
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Updated

5 years ago
Depends on: 976072
(Assignee)

Comment 16

5 years ago
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 → ---
(Assignee)

Comment 17

5 years ago
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);
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Comment 20

5 years ago
Created attachment 8381524 [details] [diff] [review]
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.
Attachment #8381524 - Flags: review?(bzbarsky)
(Assignee)

Comment 21

5 years ago
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.