Bug 24398 - BackForwardList doesn't initialize m_client, which causes a crash
Summary: BackForwardList doesn't initialize m_client, which causes a crash
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Marc-Antoine Ruel
: 24636 (view as bug list)
Depends on:
Reported: 2009-03-05 12:26 PST by Marc-Antoine Ruel
Modified: 2010-02-24 17:38 PST (History)
2 users (show)

See Also:

proposed patch (1.29 KB, patch)
2009-03-05 12:26 PST, Marc-Antoine Ruel
no flags Details | Formatted Diff | Diff
Fixed ChangeLog (1.33 KB, patch)
2009-03-09 13:25 PDT, Marc-Antoine Ruel
fishd: review-
Details | Formatted Diff | Diff
new patch file (1.32 KB, patch)
2009-03-13 11:22 PDT, Marc-Antoine Ruel
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marc-Antoine Ruel 2009-03-05 12:26:02 PST
In the chromium port, when loading test_shell with a svg file, reload the "page" will crash test_shell.
Comment 1 Marc-Antoine Ruel 2009-03-05 12:26:40 PST
Created attachment 28315 [details]
proposed patch
Comment 2 Mark Rowe (bdash) 2009-03-06 00:10:12 PST
The header for BackForwardList contains the following:

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

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.
Comment 3 Marc-Antoine Ruel 2009-03-09 12:48:25 PDT
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().
Comment 4 Darin Fisher (:fishd, Google) 2009-03-09 13:03:44 PDT
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?
Comment 5 Marc-Antoine Ruel 2009-03-09 13:25:49 PDT
Created attachment 28418 [details]
Fixed ChangeLog
Comment 6 Marc-Antoine Ruel 2009-03-09 13:27:45 PDT
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.
Comment 7 Holger Freyther 2009-03-11 02:00:24 PDT
Please don't use NULL in C++ code.
Comment 8 Darin Fisher (:fishd, Google) 2009-03-12 11:33:41 PDT
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.
Comment 9 Marc-Antoine Ruel 2009-03-13 11:22:22 PDT
Created attachment 28587 [details]
new patch file

Replace NULL with 0.
Comment 10 Darin Fisher (:fishd, Google) 2009-03-18 23:18:37 PDT
Landed as http://trac.webkit.org/changeset/41824
Comment 11 Hin-Chung Lam 2010-02-24 17:38:06 PST
*** Bug 24636 has been marked as a duplicate of this bug. ***