Bug 43152 – Assertion failure in FrameView::layout when modifying the DOM during pagehide with PageCache enabled

• Component: Page Loading
• Status: RESOLVED FIXED

[Mihai Parparita]

Created attachment 62885 [details]
Test case

The attached test case triggers this assertion failure in debug builds:

ASSERTION FAILED: m_frame->view() == this
(/Users/mihaip/Developer/source/webkit1/WebCore/page/FrameView.cpp:634 void WebCore::FrameView::layout(bool))

0   com.apple.WebCore             	0x00000001017fd345 WebCore::FrameView::layout(bool) + 449 (FrameView.cpp:634)
1   com.apple.WebCore             	0x00000001017fe5ea WebCore::FrameView::layoutTimerFired(WebCore::Timer<WebCore::FrameView>*) + 30 (FrameView.cpp:1332)
2   com.apple.WebCore             	0x0000000101801753 WebCore::Timer<WebCore::FrameView>::fired() + 113 (Timer.h:98)
3   com.apple.WebCore             	0x000000010202ce68 WebCore::ThreadTimers::sharedTimerFiredInternal() + 208 (ThreadTimers.cpp:115)
4   com.apple.WebCore             	0x000000010202cff7 WebCore::ThreadTimers::sharedTimerFired() + 25 (ThreadTimers.cpp:91)
5   com.apple.WebCore             	0x0000000101efefd4 WebCore::timerFired(__CFRunLoopTimer*, void*) + 73 (SharedTimerMac.mm:87)
6   com.apple.CoreFoundation      	0x00007fff83b67678 __CFRunLoopRun + 6488
7   com.apple.CoreFoundation      	0x00007fff83b6584f CFRunLoopRunSpecific + 575
8   com.apple.HIToolbox           	0x00007fff830e991a RunCurrentEventLoopInMode + 333
9   com.apple.HIToolbox           	0x00007fff830e971f ReceiveNextEventCommon + 310
10  com.apple.HIToolbox           	0x00007fff830e95d8 BlockUntilNextEventMatchingListInMode + 59
11  com.apple.AppKit              	0x00007fff8451e29e _DPSNextEvent + 708
12  com.apple.AppKit              	0x00007fff8451dbed -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 155
13  com.apple.Safari              	0x0000000100015c96 0x100000000 + 89238
14  com.apple.AppKit              	0x00007fff844e38d3 -[NSApplication run] + 395
15  com.apple.AppKit              	0x00007fff844dc5f8 NSApplicationMain + 364
16  com.apple.Safari              	0x0000000100009b18 0x100000000 + 39704

The triggering conditions appear to be 1) the DOM being modified in the pagehide handler 2) the page cache being enabled (adding an unload event handler to disable results in this not crashing). The fact that it's a data: URL does not seem to matter, the crash also happens when changing the location to a regular URL.

There is this comment and check in FrameView.cpp immediately after the assert:

    // This early return should be removed when rdar://5598072 is resolved. In the meantime, there is a
    // gigantic CrashTracer because of this issue, and the early return will hopefully cause graceful 
    // failure instead.  
    if (m_frame->view() != this)
        return;

That was added in http://trac.webkit.org/changeset/29878. Cc-ing Darin and Beth in case there's anything relevant to this bug at the rdar:// URL.

Bug 32752 may be related, since it's the same assert and it happens on a page transition, but I can't reproduce it. Bug 19564 also mentions this assert, and says that HTMLFrameElement09.xhtml fails because of it (and it's still disabled as of http://trac.webkit.org/changeset/34803), but I can't reproduce the assert  when running it locally (though the test doesn't pass).

[Darin Adler]

(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>.

[Mihai Parparita]

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.

[Mihai Parparita]

(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?

[Darin Adler]

(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.

[Mihai Parparita]

(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.

[Mihai Parparita]

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

[Mihai Parparita]

Created attachment 63109 [details]
Patch

[Mihai Parparita]

Darin, would you have time to review this? If not, can you suggest anyone appropriate?

[Mihai Parparita]

Simon, would you be a good person to review this? If not, do you have any suggestions as to who could look at it?

[Simon Fraser (smfr)]

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

[Mihai Parparita]

(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();

[Simon Fraser (smfr)]

So maybe the fix for this is to call unscheduleRelayout() in a new place. That layout timer should not be firing.

[Dave Hyatt]

Comment on attachment 63109 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 63109 [details]
Patch

Clearing flags on attachment: 63109

Committed r65832: <http://trac.webkit.org/changeset/65832>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.