Bug 24398 – BackForwardList doesn't initialize m_client, which causes a crash

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Marc-Antoine Ruel]

In the chromium port, when loading test_shell with a svg file, reload the "page" will crash test_shell.

[Marc-Antoine Ruel]

Created attachment 28315 [details]
proposed patch

[Mark Rowe (bdash)]

The header for BackForwardList contains the following:

#if PLATFORM(CHROMIUM)
    // Must be called before any other methods. 
    void setClient(BackForwardListClient* client) { m_client = client; }
#endif

This implies that your code is using this class incorrectly.  Of course, it may be that the existing design of the class is not ideal and it should be changed, but if that is the case the comment is no longer correct.

[Marc-Antoine Ruel]

What is happening in that particular case is that SVGImage::dataChanged() creates a new dummy Page() which doesn't have any back-forward logic. I think null-initializing is fine in this case since ~Page() always calls m_backForwardList->close(). I can update the comment in BackForwardList.h for BackForwardListClient::setClient() to
    // Must be called before any other methods except close().

[Darin Fisher (:fishd, Google)]

Comment on attachment 28315 [details]
proposed patch

>Index: WebCore/ChangeLog
...
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fix a crash when loading a non html page (like a svg file) in test_shell
>+        and reloading the page.
>+
>+        * history/BackForwardListChromium.cpp:

please add a bug link to the ChangeLog.


>Index: WebCore/history/BackForwardListChromium.cpp
...
>+    if (m_client)
>+        m_client->close();

we should probably null check each usage and probably add some ASSERTs.  wdyt?

[Marc-Antoine Ruel]

Created attachment 28418 [details]
Fixed ChangeLog

[Marc-Antoine Ruel]

I don't think it is a good idea to NULL check each functions and add ASSERTs. Having m_client null-initialized will help there and this was a very particular case. I looked at directly extracting m_client from m_page and it is very deep down multiple objects hierarchy.

[Holger Freyther]

Please don't use NULL in C++ code.

[Darin Fisher (:fishd, Google)]

Comment on attachment 28418 [details]
Fixed ChangeLog

>Index: WebCore/history/BackForwardListChromium.cpp
...
>+    , m_client(NULL)

As mentioned, this should be 0 instead of NULL.  Please post a revised patch, and then LGTM.

[Marc-Antoine Ruel]

Created attachment 28587 [details]
new patch file

Replace NULL with 0.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/41824

[Hin-Chung Lam]

*** Bug 24636 has been marked as a duplicate of this bug. ***