Bug 121284 – Settings keeps a dangling pointer to m_page after m_page has been deleted

• Component: Page Loading
• Status: NEW

[Vincent Van Den Berghe]

Created attachment 211525 [details]
Minimal patch to correct dangling pages bug

Background
----------
I’m currently embedding WebKit in a .NET application, which required me to solve a whole set of issues that are not relevant here. However, I’ve stumbled upon a scenario which I suspect is *really* problematic (but I can be wrong). Before patching anything, I’d like to submit this to greater minds for validation. The version of WebKit to which this applies is irrelevant: any recent version will do. Note: preliminary feedback from Darin Adler seems to indicate this is a platform-wide problem.

Problem
-------
When I’m finish with a WebView instance and before I’m releasing the COM object, I close it by doing a 

DestroyWindow(m_viewWindow) 

…where m_viewWindow is WebView’s view window. Note that calling WebView::close() would work just as well.
This triggers a sequence of events that ultimately causes the destructor of the page to be called:

Page::~Page()

This works well, but when stress testing this will crash the WebKit.DLL with the following stack trace:

                WebKit.dll!WebCore::setImageLoadingSettings(WebCore::Page * page)  Line 57 + 0x3 bytes      C++
               WebKit.dll!WebCore::Settings::imageLoadingSettingsTimerFired(WebCore::Timer<WebCore::Settings> * __formal)  Line 363 + 0x8 bytes C++
               WebKit.dll!WebCore::Timer<WebCore::XMLHttpRequestProgressEventThrottle>::fired()  Line 114 + 0xb bytes C++
               WebKit.dll!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 132           C++
               WebKit.dll!WebCore::TimerWindowWndProc(HWND__ * hWnd, unsigned int message, unsigned int wParam, long lParam)  Line 111              C++

Depending on the managed environment (details are irrelevant for this discussion), the crash happens either immediately or after a while. Because of the way I embed WebKit, this usually happens when the second (or third, forth…) instance is being created.
The visible cause of the crash is the call to 

WebKit.dll!WebCore::setImageLoadingSettings(WebCore::Page * page)

... with a page instance for which its destructor has already been executed.

Primary Cause
-------------
I suspect the primary cause of a crash is the pending timer whose message is still in the message queue, that is executed just after the destruction of the page, but before any pending timers are killed. This causes a delayed execution of a function on a dangling page instance.
The problem doesn’t occur when I don’t call DestroyWindow or WebView::Close(), but this leads to unacceptable memory leaks.

Workaround
----------
The minimal workaround is to add a method to WebCore::Settings:

void Settings::detachFromPage()
{
    m_page = 0;
    m_setImageLoadingSettingsTimer.stop();
}


And call this method as the first line of Page’s destructor:

Page::~Page()
{
    m_settings->detachFromPage();
    m_mainFrame->setView(0);
…


…and finally add if(page) guard in various places in WebCore::Settings (see attachment).
This doesn’t remedy the delayed execution of the timer, but solves the problem by setting the deleted instance to NULL and making sure it’s not used in the delayed call.

I have little time to follow all the rules and submit a patch and don't have the absolute latest tree anyway, but I have included a patch attachment so that anyone who is inclined to do so can take it as a starting point.

[Darin Adler]

Comment on attachment 211525 [details]
Minimal patch to correct dangling pages bug

View in context: https://bugs.webkit.org/attachment.cgi?id=211525&action=review

This patch looks great to me. What’s missing is a change log, some small stylistic changes, and either a regression test or a reason we can’t make a regression test.

> Source/WebCore/page/Settings.cpp:55
> +    if(page)

Need to put a space after "if". Probably best to use an early return here instead. Or changes this to take a Page& and put the null check at the caller.

> Source/WebCore/page/Settings.cpp:308
> +    if(m_page)

Space after "if".

> Source/WebCore/page/Settings.cpp:327
> +    if(m_page) {

Space after if.

> Source/WebCore/page/Settings.cpp:346
> +      FrameView* view = m_page->mainFrame().view();
> +      ASSERT(view);

Indentation needs to be four spaces, not two.

> Source/WebCore/page/Settings.cpp:375
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:412
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:423
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:446
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:452
> +    return m_page ? m_page->minimumTimerInterval() : 0;

Not sure that 0 is a safe value to return.

> Source/WebCore/page/Settings.cpp:467
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:473
> +    return m_page ? m_page->timerAlignmentInterval() : 0;

Not sure that 0 is a safe value to return.

> Source/WebCore/page/Settings.cpp:495
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:504
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:526
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:551
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:559
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:635
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:646
> +    if(m_page)

Space after if.

> Source/WebCore/page/Settings.cpp:658
> +    m_page = 0;

Should use nullptr.

[Darin Adler]

Maybe even qualifies as a security bug.

[Vincent Van Den Berghe]

Created attachment 211739 [details]
Revised patch

I've applied the stylistic changes requested.
The patch is still missing a changelog, because I don't know how to generate one as outlined in http://www.webkit.org/coding/contributing.html#changelogs since the scripts will gather *all* my changed files, which is not what I want: only 3 files are affected by this patch.