Make all nsINodeLists (except maybe the XBL one) inherit from nsWrapperCache

RESOLVED FIXED in mozilla6



6 years ago
6 years ago


(Reporter: bz, Assigned: peterv)



Firefox Tracking Flags

(Not tracked)



(1 attachment)

I hear tell there's a partial fix for this.
OS: Mac OS X → All
Hardware: x86 → All

Comment 1

6 years ago
Created attachment 525542 [details] [diff] [review]

Comment 2

6 years ago
Comment on attachment 525542 [details] [diff] [review]

This seems to pass on tryserver.
Attachment #525542 - Flags: review?(bzbarsky)
nsAnonymousContentList should unlink mContent.

Can you please catch me tomorrow?  I'd like to understand the exact behavior of parent stuff here, and in particular why it's ok for nsBaseContentList to QI to nsWrapperCache without reporting a useful parent.
Ah, the key is that we need the parent to ensure only one wrapper, right?  Seems like nothing ensures that right now for nsBaseContentList, so shouldn't the QI to nsWrapperCache live on nsSimpleContentList?
Or would that fail due to inheriting from nsWrapperCache anyway?

After this patch, do we still instantiate nsBaseContentList?  If so, do we only do it in places that won't get JS-wrapped or something?

Comment 6

6 years ago
(In reply to comment #5)
> After this patch, do we still instantiate nsBaseContentList?

We do not, should be impossible because its GetParentObject is pure virtual (inherited from nsINodeList). I opted for that because there was only one remaining use of it (which also wasn't exposed to JS), I think it's simpler now.
Having the QI for nsWrapperCache live on nsBaseContentList is just convenience, I can instead add it to all the derived classes if you think that makes more sense (might be a tiny bit faster too).
The GetParentObject implementations all return a sensible parent I think, let me know if you agree :-).
Comment on attachment 525542 [details] [diff] [review]

> We do not, should be impossible because its GetParentObject is pure virtual

Aha, perfect.  I'd missed that.

r=me with the comment 3 bit about unlinking, then.
Attachment #525542 - Flags: review?(bzbarsky) → review+
Peter, can you get this updated and checked in?  I'd really like to make the nodelist binding code depend on this patch....

Comment 9

6 years ago
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.