300533 - Viewmanager asserts on subframe history navigation with fastback/bfcache

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current firefox trunk build

STEPS TO REPRODUCE:
1)  Load testcase
2)  Click link in iframe
3)  Click back button

EXPECTED RESULTS:  Go back, no asserts, everything good

ACTUAL RESULTS:
###!!! ASSERTION: tried to insert view with invalid sibling: 'sibling == nsnull
|| sibling->GetParent() == parent', file
../../../mozilla/view/src/nsViewManager.cpp, line 2639

and then the iframe background paints black instead of the color it should be
(user's default color).

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, so I'm a little confused as to what's going on here.  I broke in
nsDocShell::RestorePresentation, stepped down to where we get the old root view,
and looked at what we have:

(gdb) p oldRootView
$27 = (nsIView *) 0x8a84408
(gdb) p ((nsView*)oldRootView)->mParent
$29 = (nsView *) 0x89c0eb0
(gdb) p ((nsView*)oldRootView)->mParent->mFirstChild
$30 = (nsView *) 0x8a84408
(gdb) p ((nsView*)oldRootView)->mParent->mFirstChild->mNextSibling
$31 = (nsView *) 0x88a3110
(gdb) p ((nsView*)oldRootView)->mParent->mFirstChild->mNextSibling->mNextSibling
$32 = (nsView *) 0x0
(gdb) p rootViewSibling
$33 = (nsIView *) 0x88a3110

So far so good.  Then I stepped down to where we get the new root view, and at
that point we have:

(gdb) p ((nsView*)rootViewParent)->mFirstChild
$34 = (nsView *) 0x8a84408
(gdb) p ((nsView*)rootViewParent)->mFirstChild->mNextSibling
$35 = (nsView *) 0x0

Note that for some reason oldRootView is still a child of rootViewParent.  It's
the _other_ child (the one that was the next sibling) that got removed for some
reason... And indeed, rootViewSibling is dead by this point:

(gdb) p *(nsView*)rootViewSibling
$36 = {<nsIView> = {_vptr.nsIView = 0x1, mViewManager = 0xb7ffe6e0, mParent = 0x5, 
    mWindow = 0x4, mNextSibling = 0x84b21b8, mFirstChild = 0x1c00, mClientData =
0x1, 
    mZIndex = 0, mVis = nsViewVisibility_kHide, mPosX = 137199356, mPosY =
144700800, 
    mDimBounds = {x = 0, y = 0, width = 0, height = 0}, mOpacity = 0, mVFlags = 0}, 
  mZParent = 0x0, mClipRect = 0x0, mDirtyRegion = 0x0, mChildRemoved = 0 '\0'}

Comment 2

[Boris Zbarsky [:bzbarsky]]

I did a bit more testing and I think the key problem is this:  in
RestorePresentation we have

  (gdb) p oldPresShell.mRawPtr
  $52 = (nsIPresShell *) 0x8a849d0

while if I break in PresShell::Destroy and see which presshell is destroyed when
we hide the old viewer, that gives me

  Breakpoint 6, PresShell::Destroy (this=0x8a2b188)

Not the same presshell...

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, I suspect at least part of the problem is that for a data: URI
LoadHistoryEntry nixes the current content viewer by calling
CreateAboutBlankContentViewer()....  I'll attach a testcase that shows the same
issue without data: URIs, though.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189118 [details]
Testcase

Actually, this testcase asserts in a different place.  I'll file a new bug on
it...

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, so the non-data:/etc issue is bug 300538.  And it looks like I forgot to put
the testcase in the URL field....

Comment 7

[Brian Ryner (not reading)]

Seems like something we need to fix before shipping.

Comment 8

[Brian Ryner (not reading)]

Did some debugging.  The problem seems to be that the rootViewSibling that we 
end up using belongs to the content viewer we're about to destroy.  Normally 
it would be the next view in the parent view hierarchy and would be unaffected 
by the view manager swap.  The reason it's different here is that rootView 
belongs to the transient about:blank viewer -- the blank viewer and the real 
old content viewer are both in the view tree.  Might be able to solve this by 
forcing the old content viewer to release its previousViewer before we start 
shuffling things around.

Comment 9

[Brian Ryner (not reading)]

Attachment: patch

- If we create an about:blank viewer to use between loads, then nuke mOSHE so
that we don't ever associate that content viewer with the previous page
- Destroy mContentViewer's previousViewer before grabbing the root view
sibling, as described above

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 192619 [details] [diff] [review]
patch

requesting approval... since it's not really clear from the summary, the user
impact here is potential display problems and potential memory corruption when
using fastback with data: urls.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 192619 [details] [diff] [review]
patch

This about:blank transient business should go away with the right fix to bug
293363 -- please comment something about that with an XXX or FIXME.

/be

Comment 12

[Brian Ryner (not reading)]

I don't actually think this is that specific to the transient about:blank
viewer... in any case where mContentViewer has a previousViewer, we want to get
rid of it before capturing the root view sibling.

Comment 13

[Brian Ryner (not reading)]

fixed on trunk and branch

Comment 14

[Boris Zbarsky [:bzbarsky]]

This caused bug 306580