Bug 16808 – ASSERT(m_document->frame() == m_view->frame()) failing in CachedPage::clear when running Safari's PLT

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Adam Roben (:aroben)]

ASSERT(m_document->frame() == m_view->frame()) in CachedPage::clear fails when running Safari's PLT. I believe this is due to Windows WebKit keeping FrameViews around for longer than Mac WebKit. Windows WebKit currently creates one FrameView per Frame, while Mac WebKit creates a new FrameView for each new page load in each Frame.

[Adam Roben (:aroben)]

Created attachment 18355 [details]
patch in progress

Here's a patch in progress. However, I'm hitting the assertions in FrameView::scheduleRelayout due to FrameViews in the page cache having layout scheduled for them.

[Adam Roben (:aroben)]

<rdar://5659200&5659355>

[Adam Roben (:aroben)]

See bug 13913 and bug 14794 for similar issues with the GTK port

[Kevin McCullough]

Comment on attachment 18355 [details]
patch in progress

Most of this patch looks good to me, although I'm not sure I know enough to give it an r+.

Also you said to remind you to double check if it's ok to remove the call to resetMultipleFormSubmissionProtection()

[Kevin McCullough]

Comment on attachment 18355 [details]
patch in progress

I hit the assert:  ASSERT(!m_frame->document() || !m_frame->document()->inPageCache());
Every time I navigate.  Are you not having this issue?

Comment on attachment 18355 [details]
patch in progress

 1856         frameView->attachToWindow();

I expect attachToWindow() to happen as a result of calling setWidget(). Does it not?

[Adam Roben (:aroben)]

(In reply to comment #4)
> (From update of attachment 18355 [details] [edit])
> Most of this patch looks good to me, although I'm not sure I know enough to
> give it an r+.
> 
> Also you said to remind you to double check if it's ok to remove the call to
> resetMultipleFormSubmissionProtection()

I remember now that I removed it from transitionToCommittedFromCachedPage because the Mac doesn't call it there, either.

[Adam Roben (:aroben)]

(In reply to comment #5)
> (From update of attachment 18355 [details] [edit])
> I hit the assert:  ASSERT(!m_frame->document() ||
> !m_frame->document()->inPageCache());
> Every time I navigate.  Are you not having this issue?

I am having that issue, as I said when I uploaded the patch (see comment #2).

[Adam Roben (:aroben)]

(In reply to comment #6)
> (From update of attachment 18355 [details] [edit])
>  1856         frameView->attachToWindow();
> 
> I expect attachToWindow() to happen as a result of calling setWidget(). Does it
> not?

Hm, I suppose it does: RenderPart::setWidget -> RenderWidget::setWidget -> ScrollView::addChild -> Widget::setParent -> Widget::attachToWindow. Thanks for pointing that out!

[Adam Roben (:aroben)]

(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 18355 [details] [edit] [edit])
> >  1856         frameView->attachToWindow();
> > 
> > I expect attachToWindow() to happen as a result of calling setWidget(). Does it
> > not?
> 
> Hm, I suppose it does: RenderPart::setWidget -> RenderWidget::setWidget ->
> ScrollView::addChild -> Widget::setParent -> Widget::attachToWindow. Thanks for
> pointing that out!

Ah, but we only call attachToWindow() on the main frame, and we only call setWidget() on frames that have an ownerRenderer(), so I think both calls need to remain.

(In reply to comment #10)
> Ah, but we only call attachToWindow() on the main frame, and we only call
> setWidget() on frames that have an ownerRenderer(), so I think both calls need
> to remain.

Oh, that's right. Sorry. The reason the attachToWindow() drew my attention was that I did not see a detachFromWindow() to balance it. So now the question is, should you call detachFromWindow() on the frame's current view before calling frame->setView(0)?

[Adam Roben (:aroben)]

Created attachment 18371 [details]
patch v2

With this patch I no longer hit assertions when browsing, running the PLT, or running the regression tests with the back/forward cache turned on.

[Dave Hyatt]

Comment on attachment 18371 [details]
patch v2

Make setWidget explicitly remove the widget rather than patching WidgetWin's destructor.  This way you do the removal in a cross-platform way and don't have to patch the other platforms' widget classes.

[Adam Roben (:aroben)]

Created attachment 18373 [details]
patch v3

Incorporates hyatt's comments (which actually were already implemented by a patch for bug 16456).

[Dave Hyatt]

Comment on attachment 18373 [details]
patch v3

r=me

I think it's worth explicitly mentioning in a comment that removeFromParent() is a no-op on Mac (next to the call site).

[Adam Roben (:aroben)]

(In reply to comment #15)
> (From update of attachment 18373 [details] [edit])
> r=me
> 
> I think it's worth explicitly mentioning in a comment that removeFromParent()
> is a no-op on Mac (next to the call site).

Done and landed as r29369.