213823 - [FIXr]deCOMify GetParent/GetDocument/GetBindingParent on nsIContent

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Like the summary says... Patch to make those functions return weak ptrs coming
up as soon as I finish building.

I think I caught all the callsites, including those in code that's not built on
Linux, but I can't be quite sure.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Note to self -- I had to edit up nsCSSStyleSheet a bit (in case I have to
rediff).

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

jst, would you do the honors?

Comment 3

[Johnny Stenback (:jst)]

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

- In nsGenericElement::GetBindingParent():

+    return slots->mBindingParent;
   } else {

else-after-return.

- In nsContentIterator::RebuildIndexStack():

   current = mCurNode;
   while (current && current != mCommonParent)
   {
-    if (NS_FAILED(current->GetParent(getter_AddRefs(parent))))
-      return NS_ERROR_FAILURE;
+    parent = current->GetParent();
+    
     if (!parent || NS_FAILED(parent->IndexOf(current, indx)))
       return NS_ERROR_FAILURE;

Maybe return early before this loop if !mCurNode in stead of checking for a
non-null pointer twice every iteration (in the while condition and the if
(!parent) check)? Either way...

- In nsImageLoadingContent::LoadImageWithChannel():

-  nsCOMPtr<nsIDocument> doc;
-  nsresult rv = GetOurDocument(getter_AddRefs(doc));
-  NS_ENSURE_TRUE(doc, rv);
+  nsCOMPtr<nsIDocument> doc = GetOurDocument();

Don't you still want the NS_ENSURE_TRUE(doc, ?) there? You're changing
behaviour here.

Done with the changes all the way to content/html/content, more to follow
later.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> else-after-return.

Fixed.

> Maybe return early

  current = mCurNode;
  if (!current) {
    return NS_OK;
  }
  while (current != mCommonParent)

This preserves the semantics of the original code, which would return the NS_OK
after the loop if mCurNode was null....

> Don't you still want the NS_ENSURE_TRUE(doc, ?) there?

Yes, I do.  Thank you for catching that.

Thanks for reading this thing, man.

Comment 5

[Johnny Stenback (:jst)]

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

- In nsXULElement::GetParentTree():

-      nsCOMPtr<nsIDOMXULMultiSelectControlElement> element =
do_QueryInterface(current);
-      *aTreeElement = element;
-      NS_IF_ADDREF(*aTreeElement);
-      return NS_OK;
+      return CallQueryInterface(current, aTreeElement);

This used to return NS_OK even if current wasn't an
nsIDOMXULMultiSelectControlElement, your code won't. I guess that tag check
should be enough to guarantee that, but it's not bullet proof. Maybe check the
namespace of the element too, or check that current->IsContentOfType(eXUL).

- In XULPopupListenerImpl::PreLaunchPopup():

+  nsCOMPtr<nsIDocument> document = content->GetDocument();

   // Turn the document into a XUL document so we can use SetPopupNode.
   nsCOMPtr<nsIDOMXULDocument> xulDocument = do_QueryInterface(document);
-  if (xulDocument == nsnull) {
+  if (!xulDocument) {
     NS_ERROR("Popup attached to an element that isn't in XUL!");
     return NS_ERROR_FAILURE;
   }

Nit: Do you event need |document| here? Maybe just QI on
content->GetDocument()?

- In RuleProcessorData::RuleProcessorData():

-    aContent->GetParent(&mParentContent);
+    mParentContent = aContent->GetParent();

Is it cool to make this member a weak member? If so, comment that in the
declaration.

- In SelectorMatchesTree():

-	   lastContent->GetParent(&content);
+	   content = lastContent->GetParent();
	   if (content) {
+	     NS_ADDREF(content);
	     newdata = new (curdata->mPresContext)
RuleProcessorData(curdata->mPresContext, content,
								      
curdata->mRuleWalker, &compat);
	     curdata->mParentData = newdata;	

Wow, that's messed up. Anything we can do about this?

- In nsBindingManager::UseDocumentRules():

+  *aResult = !GetOutermostStyleScope(aContent);
+;
   return NS_OK;

Uh? :-)

		 nsCOMPtr<nsIAtom> tag;
		 element->GetTag(getter_AddRefs(tag));
-		 if (tag.get() == nsXULAtoms::Template) {
+		 if (tag == nsXULAtoms::Template) {

I'm getting really tired of seeing code like this. It's most of the time an
error waiting to show up due to the lack of namespace or type check.

Maybe we should introduce an nsIContent->NameMatches(tag, ns_id)? Or maybe a
global inline somewhere? Thoughts?

Got about halfway through nsXULDocument.cpp, will continue later.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> This used to return NS_OK 

I've gone back to doing that for now (just CallQI and then return NS_OK),
because I don't know this code well enough to admix cleanup like that to this
patch...

    if (tag == nsXULAtoms::listbox) {
      CallQueryInterface(current, aTreeElement);
      // XXX returning NS_OK because that's what the code used to do;
      // is that the right thing, though?
      return NS_OK;
    }

> Nit: Do you event need |document| here?

I do not.  Fixed.

> Is it cool to make this member a weak member?

Yes.  These structs are very short-lived.  I'll comment in the struct decl to
that effect.

> Wow, that's messed up. Anything we can do about this?

Yes.  Make all the pointers weak.  At least I think that's safe... but not as
part of this patch, please.  ;)  The refcounting here is very confusing, and I'd
rather be able to focus on it when I try to clean this up.  I'll file a separate
bug on that.

> Uh? :-)

Removed extraneous ';'  ;)

> Maybe we should introduce an nsIContent->NameMatches(tag, ns_id)?

That would be great.  It could just return false for non-elements and forward to
the nodeinfo for elements...  At the moment, doing things the right way is so
painful that people just don't bother.

I think a member would be preferable to an inline that fetched the nodeinfo,
null-checked it, and then asked it.  If nothing else, it should be smaller
codesize-wise.

Comment 7

[Johnny Stenback (:jst)]

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

- In nsImageFrame::GetAnchorHREFAndTarget():

+  for (nsIContent* content = mContent->GetParent();
+	  content; content = content->GetParent()) {

Fix indentation.

- In nsHTMLFramesetFrame::GetFramesetParent():

-    nsCOMPtr<nsIContent> contentParent;
-    content->GetParent(getter_AddRefs(contentParent));
+    nsCOMPtr<nsIContent> contentParent = content->GetParent();

-    nsCOMPtr<nsIHTMLContent> contentParent2 =
-      do_QueryInterface(contentParent);
-
-    if (contentParent2) {
+    if (contentParent) {
       nsCOMPtr<nsIAtom> tag;
-      contentParent2->GetTag(getter_AddRefs(tag));
+      contentParent->GetTag(getter_AddRefs(tag));

       if (tag == nsHTMLAtoms::frameset) {

This code used to check that contentParent was an nsIHTMLContent, now it
doesn't. Maybe add a contentParent->IsContentOfType(eHTML) check to the if
(contentParent) check? Or make contentParent an nsIHTMLContent and QI on
assignment.

Gone though layout/html/forms, I'll try to finish tomorrow.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Fix indentation.

Done.

> Maybe add a contentParent->IsContentOfType(eHTML) check

Did that; I like it a lot more than QIing.

Thanks for doing this, Johnny!

Comment 9

[Johnny Stenback (:jst)]

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

- In ShouldIgnoreSelectChild():

     nsCOMPtr<nsIContent> selectContent = aContainer;
-    nsCOMPtr<nsIContent> tmpContent;

-    while (selectContent) {
-      if (containerTag == nsHTMLAtoms::select)
-	 break;
-      
-      tmpContent = selectContent;
-      tmpContent->GetParent(getter_AddRefs(selectContent));
+    while (selectContent && containerTag != nsHTMLAtoms::select) {
+      selectContent = selectContent->GetParent();
       if (selectContent)
	 selectContent->GetTag(getter_AddRefs(containerTag));
     }

Maybe make selectContent a weak pointer, and rewrite the loop to not check for
a non-null selectContent twice per iteration, i.e. something like:

    while (containerTag != nsHTMLAtoms::select) {
      selectContent = selectContent->GetParent();
      if (!selectContent)
	break;

      selectContent->GetTag(getter_AddRefs(containerTag));
    }

- In nsCSSRendering::PaintBackground():

-	 nsCOMPtr<nsIContent> parent;
-	 content->GetParent(getter_AddRefs(parent));
-	 if (parent)
+	 if (content->GetParent())
	   return;
	 else
	   color = aForFrame->GetStyleBackground();

else-after-return.

r+sr=jst with all that looked at.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Maybe make selectContent a weak pointer, and rewrite the loop

Done.

> else-after-return.

Fixed.

Comment 11

[Aleksey Nogin]

This check-in have added a "might be used uninitialized" warning to brad TBox:

+content/base/src/nsGenericElement.cpp:1867
+ `nsIContent*bindingParent' might be used uninitialized in this function

Indeed, if the "content" var is NULL on line 1871, then on line 1877 an
uninitialized pointer will be compared with NULL:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsGenericElement.cpp&rev=3.283&cvsroot=/cvsroot&mark=1867-1871,1876-1877#1865

Comment 12

[Boris Zbarsky [:bzbarsky]]

Warning fixed, but events had better have a target....

In any case, this has been checked in, tree has gone green, ports are as green
as they were before my checkin, I am hounding the Firebird redness into its grave.

This seems to have improved Tp by 0.5%-1% and improved codesize by 17k on linux,
8k on Windows.

Marking fixed.

Comment 13

[jag (Peter Annema)]

"Warning fixed, but events had better have a target...."

Sounds like an assertion we should have in the code then. Though looking at the
code, the warning comes from doing a null check on |content| and not assigning
anything into bindingParent in the else case. So the second (implied) assertion
seems to be that that QI will never fail. If that's the case, why not dump the
null check (and remove the "warning fix")?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Actually, that's a good point.  If the target is the document node (or an attr
node or something) then it will not be an nsIContent.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I notice that in some places nsCOMPtr<nsIContent> wasn't weakened to nsIContent*
where it could have been, e.g., nsContentList.cpp:846. Is this an oversight or
something that was deferred.

Comment 16

[Johnny Stenback (:jst)]

Deferred, due to the large size of this chance. At least that would be my guess.

I'm actually working on more deCOMtamination in content, so I'd rather see
changes like this held off until I'm done since I'm fixing some of those as I
go, and if someone else would go after those now I'd have merge conflicts all over.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fair enough. The only problem is that if you don't weaken the pointers as you
go, it may be hard to find the candidates again.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW I'm in the middle of nsIFrame-deCOMtaminating content/ --- hopefully it
won't stomp on you too much.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Actually, the nsContentList case was an oversight.  I attempted to convert code
that I understood well enough to convert in this patch...

jst, if I have your r/sr on the following change there:

-  nsCOMPtr<nsIContent> parent = aStartRoot->GetParent();
+  nsIContent* parent = aStartRoot->GetParent();

I'll check it in (assuming I get the reviews before Monday evening; after that
I'm gone again).