Last Comment Bug 649057 - Make all nsINodeLists (except maybe the XBL one) inherit from nsWrapperCache
: Make all nsINodeLists (except maybe the XBL one) inherit from nsWrapperCache
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Peter Van der Beken [:peterv]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-11 10:54 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2011-05-17 02:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (29.84 KB, patch)
2011-04-12 16:17 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2011-04-11 10:54:42 PDT
I hear tell there's a partial fix for this.
Comment 1 Peter Van der Beken [:peterv] 2011-04-12 16:17:52 PDT
Created attachment 525542 [details] [diff] [review]
v1
Comment 2 Peter Van der Beken [:peterv] 2011-04-13 17:30:41 PDT
Comment on attachment 525542 [details] [diff] [review]
v1

This seems to pass on tryserver.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-04-13 22:27:51 PDT
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.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-04-13 22:29:23 PDT
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?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-04-13 22:30:38 PDT
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 Peter Van der Beken [:peterv] 2011-04-18 06:09:29 PDT
(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 7 Boris Zbarsky [:bz] (still a bit busy) 2011-04-21 09:24:20 PDT
Comment on attachment 525542 [details] [diff] [review]
v1

> 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.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-04-25 13:59:34 PDT
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 Peter Van der Beken [:peterv] 2011-05-17 02:59:02 PDT
http://hg.mozilla.org/mozilla-central/rev/623563df5bd1

Note You need to log in before you can comment on or make changes to this bug.