Bug 22966 - crash when destroying a webview that opened a page containing <script> tags
Summary: crash when destroying a webview that opened a page containing <script> tags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Critical
Assignee: Gustavo Noronha (kov)
URL:
Keywords: Gtk
: 22024 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-12-22 11:27 PST by Gustavo Noronha (kov)
Modified: 2009-02-17 11:50 PST (History)
1 user (show)

See Also:


Attachments
simple test case (666 bytes, text/x-csrc)
2008-12-22 11:29 PST, Gustavo Noronha (kov)
no flags Details
simple test html (285 bytes, text/html)
2008-12-22 11:29 PST, Gustavo Noronha (kov)
no flags Details
back trace (14.52 KB, text/plain)
2008-12-22 11:32 PST, Gustavo Noronha (kov)
no flags Details
fixes the crash (1.92 KB, patch)
2009-02-10 11:23 PST, Gustavo Noronha (kov)
darin: review-
Details | Formatted Diff | Diff
proposed patch (1.54 KB, patch)
2009-02-10 15:47 PST, Gustavo Noronha (kov)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2008-12-22 11:27:07 PST
WebKit/GTK+ crashes when destroying a WebView that has opened a page containing a <script> tag. Here's how to reproduce it: open GtkLauncher, go to http://google.com/ or the attached test html. If you try to close GtkLauncher now it will crash with segmentation fault. Removing the empty <script> tag from the test HTML is enough to make the crash go away. I have tried this with SVN revision 39434.

WebKit was built using the following configure line:

./configure --prefix=/usr \
                --host=i486-linux-gnu \
                --build=i486-linux-gnu \
                --with-http-backend=soup \
                --with-font-backend=pango \
                --disable-dashboard-support \
                --enable-debug --enable-video

It doesn't seem to crash when building with debug disabled. I'm attaching test cases. I'm not really sure this problem is specific to GTK+, I didn't try with other ports, though.
Comment 1 Gustavo Noronha (kov) 2008-12-22 11:29:18 PST
Created attachment 26208 [details]
simple test case

build with:

gcc -g -o test-sigsegv test-sigsegv.c `pkg-config --cflags --libs webkit-1.0`
Comment 2 Gustavo Noronha (kov) 2008-12-22 11:29:42 PST
Created attachment 26209 [details]
simple test html
Comment 3 Gustavo Noronha (kov) 2008-12-22 11:32:26 PST
Created attachment 26210 [details]
back trace
Comment 4 Gustavo Noronha (kov) 2009-02-10 11:23:42 PST
Created attachment 27531 [details]
fixes the crash

It seems like there is this one condition in which m_group can be made NULL while m_singlePageGroup is left as it is, which causes the ASSERT on initGroup to fail, causing the crash.
Comment 5 Gustavo Noronha (kov) 2009-02-10 12:24:24 PST
Small backtrace of when setGroupName is called with an empty string:

#0  WebCore::Page::setGroupName (this=0x96a4488, name=@0xbf8c3928)
    at ../../WebCore/page/Page.cpp:230
        __PRETTY_FUNCTION__ = "void WebCore::Page::setGroupName(const WebCore::String&)"
#1  0xb717d419 in ~Page (this=0x96a4488) at ../../WebCore/page/Page.cpp:158
No locals.
#2  0xb6dc702b in webkit_web_view_dispose (object=0x96a4828)
    at ../../WebKit/gtk/webkit/webkitwebview.cpp:823
        webView = (WebKitWebView *) 0x96a4828
        priv = (WebKitWebViewPrivate *) 0x96a4870
Comment 6 Darin Adler 2009-02-10 12:41:37 PST
Comment on attachment 27531 [details]
fixes the crash

Why would we want to clear m_singlePageGroup in this case? If the group name is empty then this item should still be in the single page group, so you wouldn't want to clear it. Also, m_singlePageGroup is an OwnPtr so it will be initialized to 0, so the change to Page is unneeded and hence wrong; we left that out on purpose.

I think the right fix is to set m_group to m_singlePageGroup.get() rather than to 0.
Comment 7 Gustavo Noronha (kov) 2009-02-10 15:12:00 PST
(In reply to comment #6)
> I think the right fix is to set m_group to m_singlePageGroup.get() rather than
> to 0.

OK, that makes sense to me. I'll upload a new patch soon. Do you think a LayoutTest is required for this? I meant to write one, but didn't see it fitting anywhere, and couldn't find a good description, nor a good way to write the test.


Comment 8 Gustavo Noronha (kov) 2009-02-10 15:47:58 PST
Created attachment 27545 [details]
proposed patch

ok, this basically implements what Darin asks for, which also fixes the bug
Comment 9 Darin Adler 2009-02-10 16:05:50 PST
Comment on attachment 27545 [details]
proposed patch

r=me
Comment 10 Gustavo Noronha (kov) 2009-02-17 04:35:10 PST
Landed in r41037.
Comment 11 Jan Alonzo 2009-02-17 11:50:32 PST
*** Bug 22024 has been marked as a duplicate of this bug. ***