579830 - Convert nsCOMPtr<mozilla::dom::Element> to use nsRefPtr

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Craig Topper]

Attachment: Patch

mozilla::dom::Element isn't an interface and doesn't have an IID so shouldn't be using nsCOMPtr. Most cases its just for reference counting so nsRefPtr will work equally well.

One location does do a QI into one of these nsCOMPtr<Element> which actually QIs to nsIContent(due to it being the nearest base class with an IID) and gets typecast to an Element. While it currently works this could break in the future. In this particular case we should QI to nsINode and then call AsElement().

I have added private unimplemented copies of GetIID and kID to catch recreations of the above behavior.

Also found one instance of an nsCOMPtr<nsGenericHTMLElement> which I converted too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 458266 [details] [diff] [review]
Patch

> +++ b/content/html/content/src/nsGenericHTMLElement.cpp

Huh. I'd think that nsGenericHTMLElementTearoff needs a nonempty traverse (and maybe unlink?) implementation.  Please file a followup on that, cc peterv, request blocking2.0.

The rest looks fine to me, but I'd like Benjamin to look at the iid stuff.

Comment 2

[Craig Topper]

nsGenericHTMLElementTearoff has a proper Traverse/Unlink implemented like so:

NS_IMPL_CYCLE_COLLECTION_1(nsGenericHTMLElementTearoff, mElement)

Comment 3

[Craig Topper]

Attachment: Give Element a private IID to catch problems like this in the future

Original patch doesn't apply anymore. Went ahead and separated out this part from the rest.

Comment 4

[Craig Topper]

Attachment: Change all the nsCOMPtrs to nsRefPtr and fix the bad QI.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 460185 [details] [diff] [review]
Change all the nsCOMPtrs to nsRefPtr and fix the bad QI.

r=bzbarsky

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/99146dea1c85

Comment 7

[Benjamin Smedberg]

Comment on attachment 460184 [details] [diff] [review]
Give Element a private IID to catch problems like this in the future

I would rather not take this patch, actually, in favor of the solution in bug 514280. There are a few details to fix in that bug.

Comment 8

[Craig Topper]

(In reply to comment #7)
> Comment on attachment 460184 [details] [diff] [review]
> Give Element a private IID to catch problems like this in the future
> 
> I would rather not take this patch, actually, in favor of the solution in bug
> 514280. There are a few details to fix in that bug.

Given that I will close this bug then. I'll hang on to the private IID patch in my queue and file bugs if anything creeps back in until the other bug is checked in.