Bug 24636 - Fix crash of Chromium port in use of BackForwardList in SVG images
Summary: Fix crash of Chromium port in use of BackForwardList in SVG images
Status: RESOLVED DUPLICATE of bug 24398
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-16 17:52 PDT by Hin-Chung Lam
Modified: 2010-02-24 17:38 PST (History)
2 users (show)

See Also:


Attachments
patch (2.74 KB, patch)
2009-03-16 17:52 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
adding if (m_client) check to other methods (2.24 KB, patch)
2009-03-19 10:53 PDT, Hin-Chung Lam
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2009-03-16 17:52:09 PDT
Crash filed against Chromium: http://code.google.com/p/chromium/issues/detail?id=6869

When a SVG image is created, a WebCore::Page is created with BackForwardList but
BackForwardList::m_client is never initialized. During eviction of the cached SVG image, m_client->close() is called in BackForwardList::close() resulting in a crash. Since the WebCore::Page in SVGImage doesn't have links to the frame, there's no BackForwardListClient implementation available, so we just don't care these requests to BackForwardList and don't delegate the requests when m_client is NULL.
Comment 1 Hin-Chung Lam 2009-03-16 17:52:54 PDT
Created attachment 28673 [details]
patch
Comment 2 Hin-Chung Lam 2009-03-18 11:15:13 PDT
Review in Chromium: http://codereview.chromium.org/42265/show
Comment 3 Darin Fisher (:fishd, Google) 2009-03-19 10:29:44 PDT
maruel already fixed this:
http://trac.webkit.org/changeset/41824
Comment 4 Hin-Chung Lam 2009-03-19 10:32:45 PDT
"if (m_client)" is only performed on close(), this check should be added to other functions as well.
Comment 5 Hin-Chung Lam 2009-03-19 10:53:35 PDT
Created attachment 28755 [details]
adding if (m_client) check to other methods
Comment 6 Hin-Chung Lam 2009-03-19 10:56:16 PDT
Comment on attachment 28755 [details]
adding if (m_client) check to other methods

Since maruel has patched the class and initialize m_client as NULL, we should avoid using m_client in this case in all methods too.
Comment 7 Darin Fisher (:fishd, Google) 2009-03-19 11:11:38 PDT
This was discussed.  Please see:
https://bugs.webkit.org/show_bug.cgi?id=24398#c6
Comment 8 Eric Seidel (no email) 2009-03-26 11:32:04 PDT
Comment on attachment 28755 [details]
adding if (m_client) check to other methods

I would have just added an ASSERT(m_client) before these calls in all cases:

-    return m_client->backListCount();
+    if (m_client)
+        return m_client->backListCount();
+    ASSERT_NOT_REACHED();
+    return 0;

No need to work hard to crash only in debug mode.

The changelog should mention the bug url.

Also is there no way to test this?  There should be a layout test if at all possible.
Comment 9 Hin-Chung Lam 2010-02-24 17:38:06 PST

*** This bug has been marked as a duplicate of bug 24398 ***