Closed Bug 307394 Opened 16 years ago Closed 12 years ago

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

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: dbaron, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
See bug 261826 (and especially bug 261826 comment 4) and bug 272646
Blocks: 261826, 272646
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.
Assignee: nobody → dbaron
Assignee: dbaron → bugmail
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.
Blocks: 232990
Blocks: 386310
Blocks: 468546
Assignee: jonas → bzbarsky
Status: NEW → ASSIGNED
Attachment #357706 - Flags: superreview?(peterv)
Attachment #357706 - Flags: review?(peterv)
Attachment #357707 - Flags: superreview?
Attachment #357707 - Flags: review?
Attachment #357707 - Flags: superreview?(dbaron)
Attachment #357707 - Flags: superreview?
Attachment #357707 - Flags: review?(dbaron)
Attachment #357707 - Flags: review?
Summary: FindNextSibling and FindPreviousSibling are misused when insertion points are involved → [FIX]FindNextSibling and FindPreviousSibling are misused when insertion points are involved
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));
Attachment #357706 - Flags: superreview?(peterv)
Attachment #357706 - Flags: superreview+
Attachment #357706 - Flags: review?(peterv)
Attachment #357706 - Flags: review+
> 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 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.
Attachment #357707 - Flags: superreview?(dbaron)
Attachment #357707 - Flags: superreview+
Attachment #357707 - Flags: review?(dbaron)
Attachment #357707 - Flags: review+
> 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.
Attachment #357706 - Attachment is obsolete: true
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Depends on: 476087
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.