|Summary:||Crash in StringHash::equal due to unaligned string data|
|Product:||WebKit||Reporter:||Yong Li <yong.li.webkit>|
|Component:||WebCore Misc.||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Critical||CC:||abarth, ap, darin, dave+webkit, hausmann, joenotcharles, levin, staikos, steveblock|
|Version:||528+ (Nightly build)|
Description Yong Li 2009-11-13 09:02:44 PST
We've found a cash on espn.go.com. The reason is: 1. StringHash::equal assumes String::characters() is aligned to 4-byte boundary. 2. When PassRefPtr<StringImpl> StringImpl::create(const JSC::UString& str) uses shared buffer with UString, m_data is not guaranteed to be 4-byte aligned. because UString::Rep::data() can point to any offset of the internal buffer. The solution that Dave Tapuska suggests is: When UString::data() is not aligned to 4-byte, we just don't use the shared buffer. Anyone please give some comments?
Comment 1 David Levin 2009-11-13 12:20:48 PST
> The solution that Dave Tapuska suggests is: When UString::data() is not aligned > to 4-byte, we just don't use the shared buffer. > > Anyone please give some comments? Tricky. I created this bug unfortunately. I can see at least two solutions: 1. Dave Tapuska;s suggestion. 2. Change StringHash::Equal to use memcmp You could try each solution separately in a ship build and run drameo and see which has less of a perf impact. I suspect that #1 is the better option.
Comment 2 Yong Li 2009-11-13 12:27:49 PST
(In reply to comment #1) > > The solution that Dave Tapuska suggests is: When UString::data() is not aligned > > to 4-byte, we just don't use the shared buffer. > > > > Anyone please give some comments? > > Tricky. I created this bug unfortunately. > > I can see at least two solutions: > 1. Dave Tapuska;s suggestion. > 2. Change StringHash::Equal to use memcmp > > You could try each solution separately in a ship build and run drameo and see > which has less of a perf impact. > > I suspect that #1 is the better option. Yeah, that's what we tried, and either way can fix the problem. But we haven't run any performance test so far. BTW, the performance affect may rely on platform/compiler. memcmp (or wmemcmp?) could probably be optimized with some inline code by compiler.
Comment 3 Darin Adler 2009-11-13 12:54:36 PST
It's easy to make an alternate StringHash::equal in a way that does not depend on type punning in a non-portable way. The performance optimization of comparing four bytes at a time is not needed just to have WebKit. So I suggest starting by changing StringHash::equal to only do the optimization on platforms where it is safe. The other question, though, is whether the unaligned strings are having some sort of performance cost on platforms where the optimization is safe.
Comment 4 Joe Mason 2009-11-13 12:58:56 PST
This thread discusses the issue, but it seems their conclusion was never adopted: http://lists.macosforge.org/pipermail/webkit-dev/2009-July/008630.html
Comment 5 Yong Li 2009-11-13 13:13:57 PST
Another thing about StringImpl (not related to this bug): newUCharVector() is defined, but never used. deleteUCharVector() is used, but I don't think it ever gets called because m_data is never a standalone memory block. So need some cleanup. Probably I should raise another bug for that?
Comment 6 David Tapuska 2009-11-13 14:25:31 PST
Created attachment 43204 [details] Apply defines to StringHash::Equal
Comment 7 WebKit Commit Bot 2009-11-13 17:24:13 PST
Comment on attachment 43204 [details] Apply defines to StringHash::Equal Rejecting patch 43204 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11623 test cases. http/tests/xmlhttprequest/workers/shared-worker-close.html -> crashed Exiting early after 1 failures. 9309 tests run. 363.31s total testing time 9308 test cases (99%) succeeded 1 test case (<1%) crashed 5 test cases (<1%) had stderr output
Comment 8 Adam Barth 2009-11-15 15:34:17 PST
Comment on attachment 43204 [details] Apply defines to StringHash::Equal Clearing flags on attachment: 43204 Committed r51006: <http://trac.webkit.org/changeset/51006>
Comment 9 Adam Barth 2009-11-15 15:34:24 PST
All reviewed patches have been landed. Closing bug.
Comment 10 Alexey Proskuryakov 2009-11-20 16:07:43 PST
*** Bug 31726 has been marked as a duplicate of this bug. ***
Comment 11 Simon Hausmann 2010-04-06 07:46:57 PDT
Cherry-picked into QtWebKit for Qt 4.6 with commit e3dc4ef2b801d91e115c54f833fa7766d392ceda