Bug 53898 – Add built-in decoder for UTF-8 for improved performance

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Darin Adler]

Add built-in decoder for UTF-8 for improved performance

[Darin Adler]

Created attachment 81441 [details]
Patch

[Darin Adler]

<rdar://problem/8955789>

[WebKit Review Bot]

Attachment 81441 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:83:  Missing spaces around <=  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:83:  Missing space before ( in if(  [whitespace/parens] [5]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:86:  Missing spaces around <=  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:86:  Missing space before ( in if(  [whitespace/parens] [5]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:87:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:89:  Missing spaces around <=  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:89:  Missing space before ( in if(  [whitespace/parens] [5]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:90:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:92:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:93:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:95:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:97:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 12 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 81441 [details]
Patch

Some regression tests are failing. The patch isn’t quite ready yet.

[Darin Adler]

Created attachment 81443 [details]
Patch

[Darin Adler]

OK. New patch should be better. I had some or operators where I should have had addition operators.

[WebKit Review Bot]

Attachment 81443 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:83:  Missing spaces around <=  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:83:  Missing space before ( in if(  [whitespace/parens] [5]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:86:  Missing spaces around <=  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:86:  Missing space before ( in if(  [whitespace/parens] [5]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:87:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:89:  Missing spaces around <=  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:89:  Missing space before ( in if(  [whitespace/parens] [5]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:90:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:92:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:93:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:95:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:97:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 12 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 81443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81443&action=review

Very nice, r=me.

It certainly looks fast. Did you measure the performance against ICU?

> Source/WebCore/platform/text/TextCodecUTF8.cpp:155
> +    const uint8_t* alignedEnd = reinterpret_cast<const uint8_t*>(reinterpret_cast<uintptr_t>(end) & ~(sizeof(uintptr_t) - 1));

This would be less magical as inline function.

> Source/WebCore/platform/text/TextCodecUTF8.cpp:183
> +            if (!(reinterpret_cast<uintptr_t>(source) & (sizeof(uintptr_t) - 1))) {

As would this.

[Antti Koivisto]

...and sizeof(uintptr_t) - 1) would be less magical as a named constant.

[Darin Adler]

(In reply to comment #8)
> It certainly looks fast. Did you measure the performance against ICU?

Not yet; I plan to. There are various places where we may want to do additional tuning. I don’t know how badly the goto screws up the compiler optimization, for one thing.

> > Source/WebCore/platform/text/TextCodecUTF8.cpp:155
> > +    const uint8_t* alignedEnd = reinterpret_cast<const uint8_t*>(reinterpret_cast<uintptr_t>(end) & ~(sizeof(uintptr_t) - 1));
> 
> This would be less magical as inline function.
> 
> > Source/WebCore/platform/text/TextCodecUTF8.cpp:183
> > +            if (!(reinterpret_cast<uintptr_t>(source) & (sizeof(uintptr_t) - 1))) {
> 
> As would this.
>
> ...and sizeof(uintptr_t) - 1) would be less magical as a named constant.

I’ll make changes like these before landing.

[Darin Adler]

Committed r77819: <http://trac.webkit.org/changeset/77819>

[Csaba Osztrogonác]

(In reply to comment #11)
> Committed r77819: <http://trac.webkit.org/changeset/77819>

It broke Qt build:

In file included from ../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:27:
../../../Source/WebCore/platform/text/TextCodecUTF8.h:46: error: ‘U8_MAX_LENGTH’ was not declared in this scope
../../../Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function ‘virtual WTF::String WebCore::TextCodecUTF8::decode(const char*, size_t, bool, bool, bool&)’:
../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:180: error: ‘m_partialSequence’ was not declared in this scope
../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:187: error: ‘U8_MAX_LENGTH’ was not declared in this scope
../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:188: error: ‘completeSequence’ was not declared in this scope
../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:225: error: ‘m_partialSequence’ was not declared in this scope
../../../Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function ‘virtual WTF::CString WebCore::TextCodecUTF8::encode(const UChar*, size_t, WebCore::UnencodableHandling)’:
../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:270: error: ‘U8_APPEND_UNSAFE’ was not declared in this scope
In file included from ../../../Source/WebCore/platform/text/TextEncodingRegistry.cpp:33:
../../../Source/WebCore/platform/text/TextCodecUTF8.h:46: error: ‘U8_MAX_LENGTH’ was not declared in this scope

[WebKit Review Bot]

http://trac.webkit.org/changeset/77819 might have broken Qt Linux Release, Qt Linux Release minimal, Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, Qt Windows 32-bit Release, and Qt Windows 32-bit Debug

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #11)
> > Committed r77819: <http://trac.webkit.org/changeset/77819>
> 
> It broke Qt build:

OK, but how does reopening this bug help? I would suggest reopening only if the patch is rolled out, and otherwise using a new bug report.

> In file included from ../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:27:
> ../../../Source/WebCore/platform/text/TextCodecUTF8.h:46: error: ‘U8_MAX_LENGTH’ was not declared in this scope
> ../../../Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function ‘virtual WTF::String WebCore::TextCodecUTF8::decode(const char*, size_t, bool, bool, bool&)’:
> ../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:180: error: ‘m_partialSequence’ was not declared in this scope
> ../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:187: error: ‘U8_MAX_LENGTH’ was not declared in this scope
> ../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:188: error: ‘completeSequence’ was not declared in this scope
> ../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:225: error: ‘m_partialSequence’ was not declared in this scope
> ../../../Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function ‘virtual WTF::CString WebCore::TextCodecUTF8::encode(const UChar*, size_t, WebCore::UnencodableHandling)’:
> ../../../Source/WebCore/platform/text/TextCodecUTF8.cpp:270: error: ‘U8_APPEND_UNSAFE’ was not declared in this scope
> In file included from ../../../Source/WebCore/platform/text/TextEncodingRegistry.cpp:33:
> ../../../Source/WebCore/platform/text/TextCodecUTF8.h:46: error: ‘U8_MAX_LENGTH’ was not declared in this scope

I’ll fix this right now. Given this failure seems that Qt doesn’t use ICU and also doesn’t use UnicodeMacrosFromICU.h, which is surpising.

[Darin Adler]

I see what’s wrong. webkit-patch didn’t land the changes in JavaScriptCore!

[Darin Adler]

Landed the additional changes in <http://trac.webkit.org/changeset/77823>, but now it seems there are some failing regression tests. May need to roll this out.

[Darin Adler]

Left codec compiling, but not turned on, while I investigate regressions.

http://trac.webkit.org/changeset/77831

[Darin Adler]

Comment on attachment 81443 [details]
Patch

Clear review flag since this patch is landed.

[Adam Barth]

(In reply to comment #15)
> I see what’s wrong. webkit-patch didn’t land the changes in JavaScriptCore!

This is likely the result of Maciej's change to make webkit-patch only look at the current directory when using SVN.

[James Robinson]

Comment on attachment 81443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81443&action=review

> Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:-22
> - *  Copyright (C) 2006 George Staikos <[e-mail]>
> - *  Copyright (C) 2006 Alexey Proskuryakov <[e-mail]>
> - *  Copyright (C) 2007 Apple Computer, Inc. All rights reserved.
> - *  Copyright (C) 2008 Jürg Billeter <[e-mail]>
> - *  Copyright (C) 2008 Dominik Röttsches <[e-mail]>
> - *
> - *  This library is free software; you can redistribute it and/or
> - *  modify it under the terms of the GNU Library General Public
> - *  License as published by the Free Software Foundation; either
> - *  version 2 of the License, or (at your option) any later version.
> - *
> - *  This library is distributed in the hope that it will be useful,
> - *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> - *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - *  Library General Public License for more details.
> - *
> - *  You should have received a copy of the GNU Library General Public License
> - *  along with this library; see the file COPYING.LIB.  If not, write to
> - *  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> - *  Boston, MA 02110-1301, USA.

did you mean to take the license header out of this file?

[Darin Adler]

(In reply to comment #20)
> View in context: https://bugs.webkit.org/attachment.cgi?id=81443&action=review
> 
> > Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:-22
> > - *  Copyright (C) 2006 George Staikos <[e-mail]>
> > - *  Copyright (C) 2006 Alexey Proskuryakov <[e-mail]>
> > - *  Copyright (C) 2007 Apple Computer, Inc. All rights reserved.
> > - *  Copyright (C) 2008 Jürg Billeter <[e-mail]>
> > - *  Copyright (C) 2008 Dominik Röttsches <[e-mail]>
> > - *
> > - *  This library is free software; you can redistribute it and/or
> > - *  modify it under the terms of the GNU Library General Public
> > - *  License as published by the Free Software Foundation; either
> > - *  version 2 of the License, or (at your option) any later version.
> > - *
> > - *  This library is distributed in the hope that it will be useful,
> > - *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > - *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > - *  Library General Public License for more details.
> > - *
> > - *  You should have received a copy of the GNU Library General Public License
> > - *  along with this library; see the file COPYING.LIB.  If not, write to
> > - *  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> > - *  Boston, MA 02110-1301, USA.
> 
> did you mean to take the license header out of this file?

Yes. The copyright on that code is not held by any of those people listed in the lines I deleted, it’s held by IBM, as expressed in the line I did not delete. Since we don’t hold copyright I don’t think we can attach a license to this source file that wasn’t on the original source file.

[Darin Adler]

Created attachment 82247 [details]
Patch

[Darin Adler]

New patch improves error handling in the codec and turns it back on. Also some small coding style tweaks to the ICU codec, but no substantive changes.

[Alexey Proskuryakov]

Comment on attachment 82247 [details]
Patch

> -
> } // namespace WebCore

Please feel free to remove these noisy comments as well. This has been discussed on webkit-dev, and i don't remember any disagreement expressed.

I'm also very curious what the win over ICU is, especially on content that isn't all ASCII.

[Adam Barth]

As a matter of curiosity, would it make sense to contribute this change to ICU so that all users of the library would benefit?

[Darin Adler]

(In reply to comment #24)
> I'm also very curious what the win over ICU is, especially on content that isn't all ASCII.

I did some timings. These are on a brand new top of the line iMac so times are very short.

nytimes.com, front page

    ICU decoder: .57ms
    new decoder: .15ms
    3.8X faster

worldjournal.com, front page (most text is Chinese, includes largely ASCII markup and scripts)

    ICU decoder: .58ms
    new decoder: .17ms
    3.4X faster

wikipedia.org, front page (many non-ASCII characters; little markup and scripts)

    ICU decoder: .35ms
    new decoder: .19ms
    1.8X faster

[Darin Adler]

(In reply to comment #25)
> As a matter of curiosity, would it make sense to contribute this change to ICU so that all users of the library would benefit?

I don’t know.

I believe that much of the speed-up provided by the code I am adding here comes from removing overhead. The code wouldn’t work in ICU as-is. I’m not sure the concept of contributing the change to ICU applies in a direct fashion.

Someone familiar enough with ICU to make changes there might be able to apply some of the same techniques used to optimize our Latin-1 codec and this new UTF-8 one. I suspect, though, that the built-in codecs for these will remain at least a bit faster.

ICU has many features we do not need here, including a general purpose callback mechanism when an illegal sequence is encountered.

[Darin Adler]

Committed r78451: <http://trac.webkit.org/changeset/78451>

[Darin Adler]

(In reply to comment #24)
> (From update of attachment 82247 [details])
> > } // namespace WebCore
> 
> Please feel free to remove these noisy comments as well.

Oops, forgot.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78451 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
http/tests/xmlhttprequest/cache-override.html

[Adam Barth]

(In reply to comment #30)
> http://trac.webkit.org/changeset/78451 might have broken SnowLeopard Intel Release (Tests)
> The following tests are not passing:
> http/tests/xmlhttprequest/cache-override.html

It's a crash:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r78451%20(25286)/http/tests/xmlhttprequest/cache-override-crash-log.txt

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   libSystem.B.dylib             	0x00007fffffe00951 __memcpy + 433
1   com.apple.WebCore             	0x0000000101396685 WebCore::TextCodecUTF8::decode(char const*, unsigned long, bool, bool, bool&) + 773 (TextCodecUTF8.cpp:245)
2   com.apple.WebCore             	0x0000000101301294 WebCore::TextResourceDecoder::flush() + 100 (Vector.h:574)
3   com.apple.WebCore             	0x00000001013742ca WebCore::XMLHttpRequest::didFinishLoading(unsigned long) + 90 (XMLHttpRequest.cpp:999)
4   com.apple.WebCore             	0x00000001008c564f WebCore::DocumentThreadableLoader::loadRequest(WebCore::ResourceRequest const&, WebCore::SecurityCheckPolicy) + 623 (RetainPtr.h:69)
5   com.apple.WebCore             	0x00000001008c7450 WebCore::DocumentThreadableLoader::DocumentThreadableLoader(WebCore::Document*, WebCore::ThreadableLoaderClient*, WebCore::DocumentThreadableLoader::BlockingBehavior, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&) + 496 (DocumentThreadableLoader.cpp:101)
6   com.apple.WebCore             	0x00000001008c7b9b WebCore::DocumentThreadableLoader::loadResourceSynchronously(WebCore::Document*, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderClient&, WebCore::ThreadableLoaderOptions const&) + 75 (PassRefPtr.h:58)
7   com.apple.WebCore             	0x000000010137279e WebCore::XMLHttpRequest::createRequest(int&) + 1646 (XMLHttpRequest.cpp:673)
8   com.apple.WebCore             	0x00000001013738b5 WebCore::XMLHttpRequest::send(WTF::String const&, int&) + 69 (XMLHttpRequest.cpp:544)
9   com.apple.WebCore             	0x0000000101373d80 WebCore::XMLHttpRequest::send(int&) + 32 (RefPtr.h:58)
10  com.apple.WebCore             	0x0000000100fa22cd WebCore::JSXMLHttpRequest::send(JSC::ExecState*) + 365 (RefPtr.h:42)
11  com.apple.WebCore             	0x0000000100f9e557 WebCore::jsXMLHttpRequestPrototypeFunctionSend(JSC::ExecState*) + 119 (JSXMLHttpRequest.cpp:487)
12  ???                           	0x000059b0ca4001b8 0 + 98615842308536
13  com.apple.JavaScriptCore      	0x000000010016ed4a JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*) + 538 (WriteBarrier.h:68)

[Alejandro G. Castro]

Reverted r78451 for reason:

It is causing crashes in some bots

Committed r78465: <http://trac.webkit.org/changeset/78465>

[Alejandro G. Castro]

*** Bug 54386 has been marked as a duplicate of this bug. ***

[Alejandro G. Castro]

(In reply to comment #33)
> *** Bug 54386 has been marked as a duplicate of this bug. ***

You can check more information about the crashes in the duplicated bug 54386.

I hope it helps.

[Antti Koivisto]

WebCore::TextCodecUTF8::decode (this=0x119923b40, bytes=0x0, length=0, flush=true, stopOnError=false, sawError=@0x11992b554) at /Users/antti/webkit/OpenSource/Source/WebCore/platform/text/TextCodecUTF8.cpp:217
217	            ASSERT(count > m_partialSequenceSize);

This simple missing null check breaking one test did not require a rollout. I was just about to fix when this got rolled out.

[Alejandro G. Castro]

(In reply to comment #35)
> WebCore::TextCodecUTF8::decode (this=0x119923b40, bytes=0x0, length=0, flush=true, stopOnError=false, sawError=@0x11992b554) at /Users/antti/webkit/OpenSource/Source/WebCore/platform/text/TextCodecUTF8.cpp:217
> 217                ASSERT(count > m_partialSequenceSize);
> 
> This simple missing null check breaking one test did not require a rollout. I was just about to fix when this got rolled out.

It breaks more than 1 test and with more than 1 assertion. I tried to reproduce it without success locally, and after that I tried to find the authors in the IRC, and just after that opened a but and rolled out; because I'm sure Darin could find the issue faster than anyone, which is the better option in this cases in my opinion.

So if this is not the way to handle this situations I'm sorry, I'm ok with Anttik's proposal of just rolling out if the patch causes a major pain to other developers, we would have to decide if red bots are a major pain in this case though.

[Csaba Osztrogonác]

(In reply to comment #35)
> WebCore::TextCodecUTF8::decode (this=0x119923b40, bytes=0x0, length=0, flush=true, stopOnError=false, sawError=@0x11992b554) at /Users/antti/webkit/OpenSource/Source/WebCore/platform/text/TextCodecUTF8.cpp:217
> 217                ASSERT(count > m_partialSequenceSize);
> 
> This simple missing null check breaking one test did not require a rollout. I was just about to fix when this got rolled out.

There are other assertions here: https://bugs.webkit.org/show_bug.cgi?id=54382 .

As far as I remember, we have a "green tree policy" for core buildbots. If the regression reporter and/or the author of the original bug can't fix the redness immediately, rolling out the patch is absolutely accepted.

[Alexey Proskuryakov]

> As far as I remember, we have a "green tree policy" for core buildbots. If the regression reporter and/or the author of the original bug can't fix the redness immediately, rolling out the patch is absolutely accepted.

This is generally incorrect. If a bot can't be green most of the time, it's removed from core bots - this is why we easily accept new bots as core ones. See e.g. <http://www.mail-archive.com/[e-mail]/msg14352.html>.

[Alexey Proskuryakov]

Or bug 51804 comment 11.

[Alejandro G. Castro]

(In reply to comment #38)
> > As far as I remember, we have a "green tree policy" for core buildbots. If the regression reporter and/or the author of the original bug can't fix the redness immediately, rolling out the patch is absolutely accepted.
> 
> This is generally incorrect. If a bot can't be green most of the time, it's removed from core bots - this is why we easily accept new bots as core ones. See e.g. <http://www.mail-archive.com/[e-mail]/msg14352.html>.

Could you clarify what does this mean in this situation? What would the committer/reviewer and gardener do?

[Alexey Proskuryakov]

I didn't make any comments about this specific situation. I was answering a general statement.

[Xan Lopez]

(In reply to comment #38)
> > As far as I remember, we have a "green tree policy" for core buildbots. If the regression reporter and/or the author of the original bug can't fix the redness immediately, rolling out the patch is absolutely accepted.
> 
> This is generally incorrect. If a bot can't be green most of the time, it's removed from core bots - this is why we easily accept new bots as core ones. See e.g. <http://www.mail-archive.com/[e-mail]/msg14352.html>.

These are orthogonal issues. Bots (core or not), should be green as much as possible, and one widely accepted way of keeping them that way is to rollout changes that break them if the author is neither around nor trying to fix the issue (or if the maintainer of the bot cannot know this because that person is not online). Core bots are removed from the core set when their maintainers cannot or do not want to keep them green, which is a completely different issue.

[Csaba Osztrogonác]

(In reply to comment #38)
> > As far as I remember, we have a "green tree policy" for core buildbots. If the regression reporter and/or the author of the original bug can't fix the redness immediately, rolling out the patch is absolutely accepted.
> 
> This is generally incorrect. If a bot can't be green most of the time, it's removed from core bots - this is why we easily accept new bots as core ones. See e.g. <http://www.mail-archive.com/[e-mail]/msg14352.html>.

I agree, this is generally incorrect, but imho in this situation it's correct.
It wasn't a port specific error, but a general regression which broke GTK Linux
32-bit Debug and GTK Linux 64-bit Debug core builders. And it would have broken
Leopard Intel Debug (Tests) bot if the Leopard build wouldn't have been broken.

[Darin Adler]

Guys, with all the debating back and forth I’ve lost track of what the bug is. Where can I find information about the bug so I can re-land this?

[Darin Adler]

(In reply to comment #43)
> It wasn't a port specific error, but a general regression which broke GTK Linux
> 32-bit Debug and GTK Linux 64-bit Debug core builders. And it would have broken
> Leopard Intel Debug (Tests) bot if the Leopard build wouldn't have been broken.

How do you know that? Are you trying to say it affects all debug builds?

[Darin Adler]

I do think folks might have been a little quick to roll out. As far as I can tell nobody tried to contact me so I could fix it.

[Csaba Osztrogonác]

(In reply to comment #45)
> How do you know that? Are you trying to say it affects all debug builds?
Yes, I think, it affects all debug builds:
-  Comment #31 - Snow Leopard (relase bot)
- https://bugs.webkit.org/show_bug.cgi?id=54386 - GTK debug bot
- https://bugs.webkit.org/show_bug.cgi?id=54382 - Qt debug bot

[Alejandro G. Castro]

(In reply to comment #46)
> I do think folks might have been a little quick to roll out. As far as I can tell nobody tried to contact me so I could fix it.

I tried to contact you and ap in the IRC after checking the issue, anyway, sorry for the noise about the rollout policy.

Going back to the issue, it caused assertions in Qt, Gtk and SnowLeopard, you can check the stack traces here:

http://webkit-bots.igalia.com/amd64/svn_78458.core-when_1297677236-_-who_DumpRenderTree-_-why_11.trace.html

http://webkit-bots.igalia.com/amd64/svn_78458.core-when_1297677247-_-who_DumpRenderTree-_-why_11.trace.html

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r78464%20(25297)/http/tests/xmlhttprequest/cache-override-crash-log.txt

[Csaba Osztrogonác]

(In reply to comment #46)
> I do think folks might have been a little quick to roll out. As far as I can tell nobody tried to contact me so I could fix it.

Comment #30 was the first feedback about this bug at 2011-02-13 20:29:59 PST and Alejandro reverted the patch at 2011-02-14 04:32:17 PST. Of course, you
can't answer in the middle of the night. So I think rolling out was better 
than waiting for morning.

But now let's go to the bug. ;)

[Csaba Osztrogonác]

Created attachment 82333 [details]
Qt crash log of http/tests/security/xssAuditor/img-onerror-non-ASCII-char2-default-encoding.html (r78451) on 32 bit in debug mode

I attached Qt crash log to help debugging.

[Darin Adler]

Committed r78499: <http://trac.webkit.org/changeset/78499>

[Darin Adler]

Turns out there was just a single line of code missing. I caused the regression with a code change I made after my last run of tests.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78499 might have broken GTK Linux 64-bit Debug

[Alejandro G. Castro]

I'm afraid it is still causing crashes in GTK+, you can check them here:

http://webkit-bots.igalia.com/amd64/svn_78515.core-when_1297727606-_-who_DumpRenderTree-_-why_11.trace.html

[Csaba Osztrogonác]

And on Chromium bots: https://bugs.webkit.org/show_bug.cgi?id=54418

[Mario Sanchez Prada]

Now it's also causing crashes on SnowLeopard bots:

SnowLeopard Intel Release (WebKit2 Tests):
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20(Tests)/builds/25337/steps/layout-test/logs/stdio

SnowLeopard Intel Release (WebKit2 Tests):
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/builds/8540/steps/layout-test/logs/stdio

In case it helps, I'm pasting right here the full backtrace as got from the GTK 32-bit Debug bot:

warning: Can't read pathname for load map: Input/output error.
Core was generated by `/home/slave/webkitgtk/gtk-linux-32-debug/build/WebKitBuild/Debug/Programs/DumpR'.
Program terminated with signal 11, Segmentation fault.
#0  0xf5dd8356 in WebCore::TextCodecUTF8::decode (this=0xe81fff8, bytes=0x0, length=0, flush=true, stopOnError=false, sawError=@0xe882690) at ../../Source/WebCore/platform/text/TextCodecUTF8.cpp:217
217	            ASSERT(count > m_partialSequenceSize);

Thread 1 (Thread 32238):
#0  0xf5dd8356 in WebCore::TextCodecUTF8::decode (this=0xe81fff8, bytes=0x0, length=0, flush=true, stopOnError=false, sawError=@0xe882690) at ../../Source/WebCore/platform/text/TextCodecUTF8.cpp:217
#1  0xf5ca547e in WebCore::TextResourceDecoder::flush (this=0xe882660) at ../../Source/WebCore/loader/TextResourceDecoder.cpp:687
#2  0xf60c8432 in WebCore::XMLHttpRequest::didFinishLoading (this=0xe873368, identifier=5518) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:999
#3  0xf5c55220 in WebCore::DocumentThreadableLoader::didFinishLoading (this=0xe8860e8, identifier=5518) at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:248
#4  0xf5c55bd5 in WebCore::DocumentThreadableLoader::loadRequest (this=0xe8860e8, request=..., securityCheck=WebCore::DoSecurityCheck) at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:362
#5  0xf5c53e2f in WebCore::DocumentThreadableLoader::DocumentThreadableLoader (this=0xe8860e8, document=0xe6e53e0, client=0xe873374, blockingBehavior=WebCore::DocumentThreadableLoader::LoadSynchronously, request=..., options=...) at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:76
#6  0xf5c5365f in WebCore::DocumentThreadableLoader::loadResourceSynchronously (document=0xe6e53e0, request=..., client=..., options=...) at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:53
#7  0xf5ca5e6a in WebCore::ThreadableLoader::loadResourceSynchronously (context=0xe6e5418, request=..., client=..., options=...) at ../../Source/WebCore/loader/ThreadableLoader.cpp:69
#8  0xf60c6eed in WebCore::XMLHttpRequest::createRequest (this=0xe873368, ec=@0xffaab3c0) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:671
#9  0xf60c64c4 in WebCore::XMLHttpRequest::send (this=0xe873368, body=..., ec=@0xffaab3c0) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:543
#10 0xf60c5f68 in WebCore::XMLHttpRequest::send (this=0xe873368, ec=@0xffaab3c0) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:483
#11 0xf58144bd in WebCore::JSXMLHttpRequest::send (this=0xf1e0a240, exec=0xf1e8b080) at ../../Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:120
#12 0xf62da9ac in WebCore::jsXMLHttpRequestPrototypeFunctionSend (exec=0xf1e8b080) at DerivedSources/WebCore/JSXMLHttpRequest.cpp:486
#13 0xf253e8f6 in ?? ()
#14 0xf6466976 in JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*) () from /home/slave/webkitgtk/gtk-linux-32-debug/build/WebKitBuild/Debug/.libs/libwebkitgtk-1.0.so.0
#15 0xf64637fe in JSC::Interpreter::execute (this=0x8d8ea70, program=0xe7ec858, callFrame=0xe867bb4, scopeChain=0xe8266a0, thisObj=0xf1e00000) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:780
#16 0xf6502c09 in JSC::evaluate (exec=0xe867bb4, scopeChain=..., source=..., thisValue=...) at ../../Source/JavaScriptCore/runtime/Completion.cpp:62
#17 0xf57f65f9 in WebCore::JSMainThreadExecState::evaluate (exec=0xe867bb4, chain=..., source=..., thisValue=...) at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:54
#18 0xf581bd3a in WebCore::ScriptController::evaluateInWorld (this=0x8a386bc, sourceCode=..., world=0x8d8f450) at ../../Source/WebCore/bindings/js/ScriptController.cpp:141
#19 0xf581becc in WebCore::ScriptController::evaluate (this=0x8a386bc, sourceCode=...) at ../../Source/WebCore/bindings/js/ScriptController.cpp:164
#20 0xf584512b in WebCore::ScriptController::executeScript (this=0x8a386bc, sourceCode=...) at ../../Source/WebCore/bindings/ScriptControllerBase.cpp:59
#21 0xf59f8f05 in WebCore::ScriptElement::executeScript (this=0xe851604, sourceCode=...) at ../../Source/WebCore/dom/ScriptElement.cpp:216
#22 0xf5b678da in WebCore::HTMLScriptRunner::runScript (this=0xe7eca40, script=0xe8515c0, scriptStartPosition=...) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:317
#23 0xf5b66cf1 in WebCore::HTMLScriptRunner::execute (this=0xe7eca40, scriptElement=..., scriptStartPosition=...) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:173
#24 0xf5b5b357 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder (this=0xe8573f8) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:200
#25 0xf5b5b6ce in WebCore::HTMLDocumentParser::pumpTokenizer (this=0xe8573f8, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:250
#26 0xf5b5b193 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible (this=0xe8573f8, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:170
#27 0xf5b5bad9 in WebCore::HTMLDocumentParser::append (this=0xe8573f8, source=...) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:331
#28 0xf596046c in WebCore::DecodedDataDocumentParser::appendBytes (this=0xe8573f8, writer=0xe7a6720, data=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., length=1294, shouldFlush=false) at ../../Source/WebCore/dom/DecodedDataDocumentParser.cpp:54
#29 0xf5c572f9 in WebCore::DocumentWriter::addData (this=0xe7a6720, str=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., len=1294, flush=false) at ../../Source/WebCore/loader/DocumentWriter.cpp:201
#30 0xf5c4d5b8 in WebCore::DocumentLoader::commitData (this=0xe7a6678, bytes=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., length=1294) at ../../Source/WebCore/loader/DocumentLoader.cpp:316
#31 0xf56a68dd in WebKit::FrameLoaderClient::committedLoad (this=0x8a37400, loader=0xe7a6678, data=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., length=1294) at ../../Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:267
#32 0xf5c4d49c in WebCore::DocumentLoader::commitLoad (this=0xe7a6678, data=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., length=1294) at ../../Source/WebCore/loader/DocumentLoader.cpp:302
#33 0xf5c4d67a in WebCore::DocumentLoader::receivedData (this=0xe7a6678, data=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., length=1294) at ../../Source/WebCore/loader/DocumentLoader.cpp:328
#34 0xf5c8e408 in WebCore::MainResourceLoader::addData (this=0xe84ee90, data=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., length=1294, allAtOnce=false) at ../../Source/WebCore/loader/MainResourceLoader.cpp:158
#35 0xf5c9a249 in WebCore::ResourceLoader::didReceiveData (this=0xe84ee90, data=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., length=1294, lengthReceived=9186, allAtOnce=false) at ../../Source/WebCore/loader/ResourceLoader.cpp:279
#36 0xf5c8f548 in WebCore::MainResourceLoader::didReceiveData (this=0xe84ee90, data=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., length=1294, lengthReceived=9186, allAtOnce=false) at ../../Source/WebCore/loader/MainResourceLoader.cpp:443
#37 0xf5c9ab74 in WebCore::ResourceLoader::didReceiveData (this=0xe84ee90, data=0xe831680 "ply.xml\", true);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"If-Range\", eTag);\n", ' ' <repeats 12 times>, "req.setRequestHeader(\"Range\", \"bytes=52-59\");\n", ' ' <repeats 12 times>, "req.send(null);\n          } else if (asyncStep == 10) {\n     "..., length=1294, lengthReceived=9186) at ../../Source/WebCore/loader/ResourceLoader.cpp:430
#38 0xf56712c5 in WebCore::readCallback (source=0x9ba0598, asyncResult=0xe4d84010, data=0x0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:788
#39 0xf4c8e245 in async_ready_callback_wrapper (source_object=0x9ba0598, res=0xe4d84010, user_data=0x0) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./gio/ginputstream.c:470
#40 0xf4ca0050 in g_simple_async_result_complete (simple=0xe4d84010) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./gio/gsimpleasyncresult.c:747
#41 0xf4d74b58 in read_async_done (stream=0x9ba0598) at soup-http-input-stream.c:723
#42 0xf4d7417a in soup_http_input_stream_got_chunk (msg=0xb872f68, chunk_buffer=0xe859560, stream=0x9ba0598) at soup-http-input-stream.c:299
#43 0xf4c1508c in g_cclosure_marshal_VOID__BOXED (closure=0xe8226f0, return_value=0x0, n_param_values=2, param_values=0xb093850, invocation_hint=0xffaabf10, marshal_data=0xf4d74090) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./gobject/gmarshal.c:568
#44 0xf4c04d02 in g_closure_invoke (closure=0xe8226f0, return_value=0x0, n_param_values=2, param_values=0xb093850, invocation_hint=0xffaabf10) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./gobject/gclosure.c:767
#45 0xf4c1e48d in signal_emit_unlocked_R (node=<value optimized out>, detail=<value optimized out>, instance=0xb872f68, emission_return=0x0, instance_and_params=0xb093850) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./gobject/gsignal.c:3252
#46 0xf4c1fafc in g_signal_emit_valist (instance=0xb872f68, signal_id=224, detail=0, var_args=0xffaac0e0 "\231a\327\364\244\274\331\364\270\341\252\377'\303\327\364h/\207\v`\225\205\016\016\005") at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./gobject/gsignal.c:2983
#47 0xf4c20212 in g_signal_emit (instance=0xb872f68, signal_id=224, detail=0) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./gobject/gsignal.c:3040
#48 0xf4d761c6 in soup_message_got_chunk (msg=0xb872f68, chunk=0xe859560) at soup-message.c:1011
#49 0xf4d7c327 in read_body_chunk (msg=<value optimized out>) at soup-message-io.c:487
#50 0xf4d7ca7f in io_read (sock=0x8e6ed68, msg=0xb872f68) at soup-message-io.c:958
#51 0xf4d7d500 in io_unpause_internal (msg=0xb872f68) at soup-message-io.c:1207
#52 0xf4b504e1 in g_idle_dispatch (source=0xe83ab28, callback=0xbbadbeef, user_data=0xb872f68) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./glib/gmain.c:4536
#53 0xf4b527a5 in g_main_dispatch (context=0x89f0e20) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./glib/gmain.c:2440
#54 g_main_context_dispatch (context=0x89f0e20) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./glib/gmain.c:3013
#55 0xf4b56d58 in g_main_context_iterate (context=0x89f0e20, block=<value optimized out>, dispatch=1, self=0x89cb0a8) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./glib/gmain.c:3091
#56 0xf4b57297 in g_main_loop_run (loop=0xe754888) at /build/buildd-glib2.0_2.27.91-1-i386-BodI3i/glib2.0-2.27.91/./glib/gmain.c:3299
#57 0xf5014dc9 in IA__gtk_main () at /build/buildd-gtk+2.0_2.20.1-1-i386-Ixfflh/gtk+2.0-2.20.1/gtk/gtkmain.c:1219
#58 0x0806128e in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:659
#59 0x080609ad in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:473
#60 0x08062796 in main (argc=2, argv=0xffaaeda4) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1111


As for GTK, I'm skipping the test while it's not fixed.

[Antti Koivisto]

I fixed the crash in http/tests/xmlhttprequest/cache-override.html

There is a followup in https://bugs.webkit.org/show_bug.cgi?id=54444 which Darin might want consider.

[Antti Koivisto]

In http://trac.webkit.org/changeset/78541 I mean.