Bug 15584 - REGRESSION(r26696): GtkLauncher segfaults on WebCore::WidthIterator::advance
Summary: REGRESSION(r26696): GtkLauncher segfaults on WebCore::WidthIterator::advance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Linux
: P2 Normal
Assignee: Mark Rowe (bdash)
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-10-20 18:58 PDT by Jan Alonzo
Modified: 2007-10-21 09:42 PDT (History)
2 users (show)

See Also:


Attachments
segfault backtrace (6.07 KB, text/plain)
2007-10-20 19:00 PDT, Jan Alonzo
no flags Details
Reduction (114 bytes, text/html)
2007-10-20 19:04 PDT, Mark Rowe (bdash)
no flags Details
Patch (1.61 KB, patch)
2007-10-20 19:45 PDT, Mark Rowe (bdash)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Alonzo 2007-10-20 18:58:39 PDT
GtkLauncher crashes on WebCore::WidthIterator::advance with a segmentation fault. This happens on select sites like google.com, wikipedia, but it doesn't seem to crash on http://planet.gnome.org.

Steps to reproduce:

1) Launch GtkLauncher with 
  ./WebKitBuild/Debug/WebKitTools/GtkLauncher/GtkLauncher

2) Crash.
Comment 1 Jan Alonzo 2007-10-20 19:00:47 PDT
Created attachment 16751 [details]
segfault backtrace

backtrace
Comment 2 Mark Rowe (bdash) 2007-10-20 19:04:14 PDT
Created attachment 16752 [details]
Reduction

I don't see the crash on launch as described, but I can reproduce this when searching Google for "bdash".  I've attached a reduction of the page that demonstrates the crash.  It appears to be a single Unicode character that is causing the problem.
Comment 3 Mark Rowe (bdash) 2007-10-20 19:15:07 PDT
This was introduced in http://trac.webkit.org/projects/webkit/changeset/26696.
Comment 4 Mark Rowe (bdash) 2007-10-20 19:45:05 PDT
Created attachment 16755 [details]
Patch
Comment 5 Darin Adler 2007-10-20 19:47:59 PDT
Comment on attachment 16755 [details]
Patch

Good fix. But we really should structure this so we don't call glyphDataForCharacter twice; it can be an expensive operation. If references make this too tricky, you can use a const GlyphData*.
Comment 6 Dave Hyatt 2007-10-20 19:49:37 PDT
Comment on attachment 16755 [details]
Patch

r=me, although darin should maybe look at this when he gets a chance.
Comment 7 Mark Rowe (bdash) 2007-10-20 19:51:47 PDT
Landed in r26837.
Comment 8 Mark Rowe (bdash) 2007-10-20 19:58:04 PDT
Darin, GlyphPage::glyphDataForCharacter is always an array lookup so I wouldn't consider it an expensive operation.  I landed it after Dave reviewed it on IRC, but I can go ahead and make the change you suggested if you would like.
Comment 9 Darin Adler 2007-10-21 09:42:07 PDT
(In reply to comment #8)
> Darin, GlyphPage::glyphDataForCharacter is always an array lookup so I wouldn't
> consider it an expensive operation.  I landed it after Dave reviewed it on IRC,
> but I can go ahead and make the change you suggested if you would like.

OK. I can live with this the way it is, I guess.