300538 - [FIXr]Asserts on subframe navigation with bfcache

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:  Load testcase in URL bar and follow directions

ACTUAL RESULTS:
###!!! ASSERTION: error in InsertChild: 'newRootView->GetNextSibling() ==
rootViewSibling', file ../../../mozilla/docshell/base/nsDocShell.cpp, line 5255

EXPECTED RESULTS: no asserts

I suspect the problem here is that we're passing PR_TRUE for the aAfter
parameter to InsertChild, but since the sibling we're passing is the old
nextSibling, we want to pass PR_FALSE...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: This does seem to fix it...

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 189121 [details] [diff] [review]
This does seem to fix it...

I don't think this is right.  rootViewSibling is what we want the _next_
sibling of newRootView to be.  The aAfter parameter of InsertChild refers to
the document order, so passing true means that the view will be inserted
_before_ rootViewSibling, which is what we want.  The naming gets a little
confusing, so please double-check me on that.  At this point, though, I'm
guessing the assertion is caused by something else...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm...  roc, are views stored in document order, or reversed?  The comment on
nsIView::GetFirstChild indicates the former, while the comments in
nsViewManager::InsertChild indicate the latter.  In any case, the case we're
looking at here (in that testcase) is that rootViewSibling is null.  So we'll
hit the following code in nsViewManager:

2649       if (nsnull == aSibling) {
2650         if (aAfter) {
2651           // insert at end of document order, i.e., before first view
2652           // this is the common case, by far
2653           parent->InsertChild(child, nsnull);

This inserts |child| as the mFirstChild of |parent|; if |parent| had any
children, then child->GetNextSibling() will be non-null after this call, which
is what triggers our assert.

Perhaps the assert itself is what's wrong?  If views are stored in reverse
order, then I'd expect that rootViewSibling->GetNextSibling() would be the new
root view...

Comment 4

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

(In reply to comment #3)
> Hmm...  roc, are views stored in document order, or reversed?

Reversed.

> The comment on
> nsIView::GetFirstChild indicates the former, while the comments in
> nsViewManager::InsertChild indicate the latter.  In any case, the case we're
> looking at here (in that testcase) is that rootViewSibling is null.  So we'll
> hit the following code in nsViewManager:
> 
> 2649       if (nsnull == aSibling) {
> 2650         if (aAfter) {
> 2651           // insert at end of document order, i.e., before first view
> 2652           // this is the common case, by far
> 2653           parent->InsertChild(child, nsnull);
> 
> This inserts |child| as the mFirstChild of |parent|; if |parent| had any
> children, then child->GetNextSibling() will be non-null after this call, which
> is what triggers our assert.

InsertChild(.., child, sib, PR_TRUE) inserts the child after sib in content
order, which is before sib in view order. BUT when sib is null it inserts at the
end of the the document order, i.e., first in view order. So I think when
oldRootSibling is null, the old root as at the end of the view list --- last in
content order --- and you want to call InsertChild(.., child, nsnull, PR_FALSE)
in that case. Sorry that this API is a broken.

> Perhaps the assert itself is what's wrong?  If views are stored in reverse
> order, then I'd expect that rootViewSibling->GetNextSibling() would be the new
> root view...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch per roc's explanation

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189143 [details] [diff] [review]
Patch per roc's explanation

Requesting approval for 1.8b4.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed.