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.
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.
<rdar://5659200&5659355>
See bug 13913 and bug 14794 for similar issues with the GTK port
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()
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?
(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.
(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).
(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!
(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)?
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.
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.
Created attachment 18373 [details] patch v3 Incorporates hyatt's comments (which actually were already implemented by a patch for bug 16456).
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).
(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.