Summary: | Assertion failure in FrameView::layout when modifying the DOM during pagehide with PageCache enabled | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihai Parparita <mihaip> | ||||||
Component: | Page Loading | Assignee: | Mihai Parparita <mihaip> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aroben, bdakin, commit-queue, darin, simon.fraser | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Mihai Parparita
2010-07-28 15:19:11 PDT
(In reply to comment #0) > Cc-ing Darin and Beth in case there's anything relevant to this bug at the rdar:// URL. That Radar is marked as a duplicate of the bug fixed in <http://trac.webkit.org/changeset/46136>. An additional wrinkle: this only happens when the attached test case is served locally (from a file:/// URL), if viewing it when the file served from Bugzilla then the assert is not triggered. (In reply to comment #1) > (In reply to comment #0) > > Cc-ing Darin and Beth in case there's anything relevant to this bug at the rdar:// URL. > > That Radar is marked as a duplicate of the bug fixed in <http://trac.webkit.org/changeset/46136>. Thanks. So the comment/early return in FrameView.cpp should be removed? (In reply to comment #3) > So the comment/early return in FrameView.cpp should be removed? If the comment is correct, the code can be removed. But as I see it, if the comment was correct, then we would not be hitting the assertion! I am not yet sure we can remove this. (In reply to comment #4) > If the comment is correct, the code can be removed. > > But as I see it, if the comment was correct, then we would not be hitting the assertion! I am not yet sure we can remove this. OK, makes sense :) Darin Fisher and I looked a bit at this, and he suspects that the layout timer list is not being cleared when navigating away from pages in some cases. I'll investigate more. As best as I can tell, the code path for invoking pagehide event handlers when the page cache is not enabled is: - FrameLoader::commitProvisionalLoad - FrameLoader::transitionToCommitted - FrameLoader::closeURL - FrameLoader::stopLoading(UnloadEventPolicyUnloadAndPageHide) - DOMWindow::dispatchEvent(pageHideEvent) - WebFrameLoaderClient::transitionToCommittedForNewPage - Frame::SetView(0) - FrameView::unscheduleRelayout While when the page cache is enabled, it is: - FrameLoader::commitProvisionalLoad - PageCache::add - CachedPage::create - CachedFrame::create - FrameLoader::stopLoading(UnloadEventPolicyUnloadAndPageHide) - DOMWindow::dispatchEvent(pageHideEvent) - WebFrameLoaderClient::transitionToCommittedForNewPage - Frame::SetView(0) (in the page cache case we call stopLoading with UnloadEventPolicyUnloadOnly in FrameLoader::closeURL, which is why they don't get invoked there) The other difference is that later on, in Frame::SetView(0) (see how we get there above), there is this snippet: 235 // Detach the document now, so any onUnload handlers get run - if 236 // we wait until the view is destroyed, then things won't be 237 // hooked up enough for some JavaScript calls to work. 238 if (!view && m_doc && m_doc->attached() && !m_doc->inPageCache()) { 239 // FIXME: We don't call willRemove here. Why is that OK? 240 m_doc->detach(); 241 if (m_view) 242 m_view->unscheduleRelayout(); 243 } If the document is in the page cache, not only do we not detach the document, but we don't unschedule pending layouts either. The assert goes away if the unscheduleRelayout call is moved to be outside the attached/page cache check. Generally that seems like a good thing (we don't care about pending layouts in any case if we're about to switch away from that view), but I don't know this code well enough. This code was built up over time, so I'm not clear about the intention either (e.g. the comment about unload handlers seems wrong, since they don't get invoked by Document::detach). The relevant changes in this area are: http://trac.webkit.org/changeset/19373 http://trac.webkit.org/changeset/25395 http://trac.webkit.org/changeset/25397 Created attachment 63109 [details]
Patch
Darin, would you have time to review this? If not, can you suggest anyone appropriate? Simon, would you be a good person to review this? If not, do you have any suggestions as to who could look at it? Is m_frame->view() null in this case? It seems that is something is trying to do layout when in the page cache, we need to decid if we should not do the layout, and leave the document in the "needs layout" state until it comes out of the page cache (In reply to comment #10) > Is m_frame->view() null in this case? No, it's the previous page's view (CachedFrame keeps a reference to it). > It seems that is something is trying to do layout when in the page cache, we need to decid if we should not do the layout, and leave the document in the "needs layout" state until it comes out of the page cache I don't think this is necessary, when restoring from the page cache we always do a layout. From http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp#L1875: if (m_loadingFromCachedPage) { ... // Force a layout to update view size and thereby update scrollbars. m_frame->view()->forceLayout(); So maybe the fix for this is to call unscheduleRelayout() in a new place. That layout timer should not be firing. Comment on attachment 63109 [details]
Patch
r=me
Comment on attachment 63109 [details] Patch Clearing flags on attachment: 63109 Committed r65832: <http://trac.webkit.org/changeset/65832> All reviewed patches have been landed. Closing bug. |