307394 - [FIX]FindNextSibling and FindPreviousSibling are misused when insertion points are involved

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There's a comment at one of the callers reflecting the problem, but I think I
saw it in the debugger a few days ago relating to the other caller
(NeedSpecialFrameReframe), I think while debugging
http://xulplanet.com/testcases/example-viewer.xul plus bug 306663's bookmarklet.
 NeedSpecialFrameReframe itself, fwiw, is completely incomprehensible, and I'm
curious to know what it's supposed to be doing.

In any case, when XBL is involved, nsCSSFrameConstructor::FindPreviousSibling
and nsCSSFrameConstructor::FindNextSibling can do a |seek| on a ChildIterator
where the index has no relationship to the child list that the seek is being
done in -- basically whenever the ChildIterator has a non-null mNodes.  We need
to figure out what the correct behavior is here.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Another complaint about these two functions:  the function IsValidSibling, of
which they're the only caller, seems to be grossly misused.  I don't see why it
would ever make sense to *skip* a node that's not a "valid sibling".  Instead,
one should grovel either up or down the tree through the right table-related
pseudo-elements.

Comment 2

[Boris Zbarsky [:bzbarsky]]

See bug 261826 (and especially bug 261826 comment 4) and bug 272646

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, I think the real long-term solution here is to implement the XBL DOM APIs,
implement frame construction on top of them and move XBL out of the frame
constructor, and have an entirely separate layer that fixes up the
nsIDocumentObserver notifications before passing them on to the frame constructor.

Comment 4

[Bernd]

I am a big fan of IsValidSibling https://bugzilla.mozilla.org/show_bug.cgi?id=341858#c21. The function is wrong on two accounts:
- it is called before xbl expansion
- it neglects tag based frame creation and looks only at the display type.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Make nsINodeList able to hand back content indices

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Build on that to fix this bug

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 357706 [details] [diff] [review]
Make nsINodeList able to hand back content indices

>diff --git a/content/base/src/nsContentList.cpp b/content/base/src/nsContentList.cpp

>+PRInt32
>+nsContentList::IndexOf(nsIContent* aContent)
>+{
>+  return IndexOf(aContent, PR_TRUE);

Do we need this one? Isn't the nsBaseContentList one enough?

>diff --git a/content/xbl/src/nsBindingManager.cpp b/content/xbl/src/nsBindingManager.cpp

>@@ -662,17 +684,17 @@ nsBindingManager::ResolveTag(nsIContent*
> 
> nsresult
> nsBindingManager::GetContentListFor(nsIContent* aContent, nsIDOMNodeList** aResult)
> { 
>   // Locate the primary binding and get its node list of anonymous children.

Is this comment correct? GetAnonymousNodesInternal has the same :-/.

> nsBindingManager::GetAnonymousNodesFor(nsIContent* aContent,
>                                        nsIDOMNodeList** aResult)
> {
>   PRBool dummy;
>-  return GetAnonymousNodesInternal(aContent, aResult, &dummy);
>+  *aResult = GetAnonymousNodesInternal(aContent, &dummy);
>+  NS_IF_ADDREF(*aResult);

Maybe
  NS_IF_ADDREF(*aResult = GetAnonymousNodesInternal(aContent, &dummy));

> nsBindingManager::GetXBLChildNodesFor(nsIContent* aContent, nsIDOMNodeList** aResult)
> {
>+  *aResult = GetXBLChildNodesFor(aContent);
>+  NS_IF_ADDREF(*aResult);

Maybe
  NS_IF_ADDREF(*aResult = GetXBLChildNodesFor(aContent));

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Do we need this one? Isn't the nsBaseContentList one enough?

nsContentList overrides the other IndexOf signature, so we do need this one to avoid the hiding warnings if nothing else...

> Is this comment correct? GetAnonymousNodesInternal has the same :-/.

Not really, no.  In either place.  I'll rewrite them to make more sense.

>  NS_IF_ADDREF(*aResult = GetAnonymousNodesInternal(aContent, &dummy));

Will do.  Same for the other NS_IF_ADDREF.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 357707 [details] [diff] [review]
Build on that to fix this bug

>-      // If there is no previous sibling, then find the frame that follows
>-      if (! prevSibling) {
>-        nextSibling = (aIndexInContainer >= 0)
>-          ? FindNextSibling(container, aIndexInContainer, aChild)
>-          : FindNextAnonymousSibling(aContainer, aChild);
>-      }

Why did you remove this for the letter frames case but not the main case?  (Then again, why does the letter frames case already differ by not having the no-next-sibling handling?)


Otherwise, looks good; r+sr=dbaron.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Why did you remove this for the letter frames case but not the main case? 

It was unused for letter frames (and that's why there's an indentation change up at the top: I changed the scoping for nextSibling to match where it's actually used).  For the main case it's used to find the right parent frame (the parent of the nextSibling), but for letter frames we already know what our parent should be: the parent of the letter frame if that was the parent we found before, and the parent we found before otherwise.

All that stuff with nextSibling and !nextSibling is just to find the right continuation of the parent to use, basically...  This code is sorta assuming that the continuation to use doesn't change when letter frames are removed.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: nsINodeList change updated to comments

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed:

http://hg.mozilla.org/mozilla-central/rev/c9a94760bc2a
http://hg.mozilla.org/mozilla-central/rev/0d5743c16ba8

and then to fix Windows bustage:

http://hg.mozilla.org/mozilla-central/rev/b9f613acb0c3

This is fixed.

No tests here, but various tests in the dependent bugs.