232417 - deCOMtaminate nsIHTMLDocument

Status: RESOLVED FIXED

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

Description

[Pratik]

We need to deCOMtaminate nsIHTMLDocument. I'll post a patch soon.

Comment 1

[Pratik]

Attachment: Patch, v1

Here's the patch. Works on Linux.

Comment 2

[Pratik]

Attachment: Path, v2

New patch. Addresses biesi's comments on IRC. Callers of GetImageMap now do an
addRef after calling it since I got rid of the addRef from GetImageMap().

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 140420 [details] [diff] [review]
Path, v2

- In nsHTMLImageAccessible.cpp:

-      htmlDoc->GetImageMap(mapElementName, getter_AddRefs(mMapElement));      
+      mMapElement = htmlDoc->GetImageMap(mapElementName);
+      NS_ADDREF(mMapElement);

mMapElement is an nsCOMPtr, no need to (and you can't) addref it.

- In content/base/src/nsContentUtils.cpp:

-	   htmlDocument->GetNumFormsSynchronous(&index);
+	   index = htmlDocument->GetNumFormsSynchronous();
	   index--;

How about just:

+	   index = htmlDocument->GetNumFormsSynchronous() - 1;

and avoiding index--?

- In content/base/src/nsRange.cpp:

-	 nsCompatibility compatMode;
-	 htmlDoc->GetCompatibilityMode(compatMode);
+	 nsCompatibility compatMode = htmlDoc->GetCompatibilityMode();
	 switch (compatMode) {

Do we need compatMode here? You can probably just switch on
htmlDoc->GetCompatibilityMode().

- In nsHTMLDocument::RemoveImageMap():

+  if (nsnull != aMap) {
   mImageMaps.RemoveObject(aMap);

Is that null check needed? Seems like it'd be safe to call
mImageMpas.RemoveObject(aMap) even if aMap is null.

- In nsHTMLDocument::GetFormControlElements():

   elements = new nsContentList(this, MatchFormControls, nsString());
-  NS_ENSURE_TRUE(elements, NS_ERROR_OUT_OF_MEMORY);
+  //NS_ENSURE_TRUE(elements, NS_ERROR_OUT_OF_MEMORY);

Just remove this check, and document in nsIHTMLDocument that this method
returns null only when out of memory (and thus it's up to the callers to check
if they care). You should also make the nsContentUtils code that calls this
method return NS_ERROR_OUT_OF_MEMORY if this method returns null.

- In nsHTMLValue.cpp:

-      nsCompatibility mode;
-      doc->GetCompatibilityMode(mode);
+      nsCompatibility mode = doc->GetCompatibilityMode();
       inNavQuirksMode = (mode == eCompatibility_NavQuirks);

No need for the local variable mode here.

- In nsImageMapUtils.cpp:

-    htmlDoc->GetImageMap(usemap, aMap);
+    *aMap = htmlDoc->GetImageMap(usemap);
+    NS_ADDREF(*aMap);

Shouldn't that be an NS_IF_ADDREF()?

Fix those minor issues and this should be good to go in!

Comment 4

[Pratik]

Attachment: patch, v3

New patch. Addresses jst's comments. Also, I noticed that I wasn't doing a
NS_ADDREF after calling GetFormControlElements() (which used to do an AddRef
before). Fixed that.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 140514 [details] [diff] [review]
patch, v3

- In nsContentUtils.cpp:

+    nsIDOMNodeList *formControls = htmlDocument->GetFormControlElements();
+    NS_ENSURE_TRUE(formControls, NS_ERROR_OUT_OF_MEMORY);
+    NS_ADDREF(formControls);

Sorry, I mislead you here. This will cause a leak, see comment below.

- In nsHTMLDocument::GetFormControlElements():

-  nsContentList* elements = nsnull;
-  elements = new nsContentList(this, MatchFormControls, nsString());
-  NS_ENSURE_TRUE(elements, NS_ERROR_OUT_OF_MEMORY);
-
-  *aReturn = elements;
-  NS_ADDREF(*aReturn);
-
-  return NS_OK;
+  return new nsContentList(this, MatchFormControls, nsString());

I didn't realize that this call always returned a new list, and since that's
the case we need to make this call return a strong reference to keep the
ownership model clean. I made this return already_AddRefed<nsIDOMNodeList> and
made this method addref the new list that it's passing back. That way it's
clear that the caller needs to release it, and when it does, the list will go
away.

Thanks for the patch! I'll fix that above issue and check this in for you.

Comment 6

[Johnny Stenback (:jst)]

Fix checked in, keep up the good work!