Bug 100577 – String::fromUTF8() should take advantage of the ASCII check in convertUTF8ToUTF16()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

By taking advantage of the ASCII check recently added to convertUTF8ToUTF16(), fromUTF8() could create an 8 bit string where possible.

[Michael Saboff]

Created attachment 171060 [details]
Patch

[WebKit Review Bot]

Comment on attachment 171060 [details]
Patch

Clearing flags on attachment: 171060

Committed r132736: <http://trac.webkit.org/changeset/132736>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #2)
> (From update of attachment 171060 [details])
> Clearing flags on attachment: 171060
> 
> Committed r132736: <http://trac.webkit.org/changeset/132736>

It broke all plugin tests on WK2 platforms.
- GTK: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20WK2%20%28Tests%29/r132737%20%282830%29/results.html
- Qt: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/r132762%20%2810200%29/results.html

Could you check and fix it, please?

[WebKit Review Bot]

Re-opened since this is blocked by bug 100652

[Csaba Osztrogonác]

(In reply to comment #5)
> Re-opened since this is blocked by bug 100652

Rollout landed in http://trac.webkit.org/changeset/132784

[Michael Saboff]

Created attachment 173660 [details]
Patch with ASSERTS to find GTK and Qt failures

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 171060 [details] [details])
> > Clearing flags on attachment: 171060
> > 
> > Committed r132736: <http://trac.webkit.org/changeset/132736>
> 
> It broke all plugin tests on WK2 platforms.
> - GTK: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20WK2%20%28Tests%29/r132737%20%282830%29/results.html
> - Qt: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/r132762%20%2810200%29/results.html
> 
> Could you check and fix it, please?

Could you try the attached patch built debug with the WK2 plugin tests and report any failures?

[Csaba Osztrogonác]

Sure. I'm on it.

[Csaba Osztrogonác]

I ran plugin tests in debug mode with your patch on Qt-WK2, but I 
still get same results as I mentioned previously and no assertion.

[Michael Saboff]

(In reply to comment #9)
> I ran plugin tests in debug mode with your patch on Qt-WK2, but I 
> still get same results as I mentioned previously and no assertion.

I'm currently at a loss as to why the plugin tests are failing on just Qt and GTK.

Let me post a patch that will output data from within fromUTF8().  If you run a couple of tests and provide the output, that would be helpful.

[Michael Saboff]

Created attachment 174252 [details]
Updated patch with debugging

I think I may have found the issue, but here is a patch with debugging.  I think fromUTF8() isn't handling the case of a valid stringStart but a 0 length.  I added a check for that in this patch.

Please apply this patch and build it debug.  Try LayoutTests/http/tests/plugins/get-url.html and attach the created get-url-stderr.txt.

Then if you could, edit Source/WTF/wtf/text/WTFString.cpp and comment out the line "#define DEBUG_FROMUTF8 1" (~line 862 in my version).  Rebuild and retest.

Thanks in advance.

[Csaba Osztrogonác]

Created attachment 174412 [details]
http/tests/plugins/get-url-stderr.txt

[Csaba Osztrogonác]

After commenting out "#define DEBUG_FROMUTF8 1" line, plugin tests still fail. :(

[Michael Saboff]

Created attachment 174447 [details]
Another Debug Patch

Thanks for the last output.  I still think there is an issue with length == 0.  I modified what happens in that case and changed the debugging a little.  Please try this one as well, including with the "#define DEBUG_FROMUTF8 1" removed.

Thanks.

[Csaba Osztrogonác]

Let me see.

[Csaba Osztrogonác]

Created attachment 174467 [details]
http/tests/plugins/get-url-stderr.txt

[Csaba Osztrogonác]

(In reply to comment #16)
> Created an attachment (id=174467) [details]
> http/tests/plugins/get-url-stderr.txt

and plugin tests still fail unfortunately :(

[Michael Saboff]

(In reply to comment #16)
> Created an attachment (id=174467) [details]
> http/tests/plugins/get-url-stderr.txt

I've spent some time trying to figure things out from the last debug information.  I still don't understand what the issue is.  At this point, I suspect the plugin process is not getting started, possible due to the path.  Could you spend some time trying to figure out what the issue is?

Let me know if you need anything.

Thanks in advance.

[Balazs Kelemen]

I think I found the issue: bug 102482

[Balazs Kelemen]

Plugin tests works fine with Qt with this patch and my proposed fix.

[Balazs Kelemen]

Btw probably we should have an easy way to force making a 16 bit string null terminated string from a 8 bit source. Probably that would work:

static String make16BitFrom8BitSource(const char* s)
{
    return make16BitFrom8BitSource(reinterpret_cast<const LChar*>(s), strlen(s) + 1);
}

[Michael Saboff]

(In reply to comment #21)
> Btw probably we should have an easy way to force making a 16 bit string null terminated string from a 8 bit source. Probably that would work:
> 
> static String make16BitFrom8BitSource(const char* s)
> {
>     return make16BitFrom8BitSource(reinterpret_cast<const LChar*>(s), strlen(s) + 1);
> }

If you need it, add the function.  Note that I think you also need to set s_hashFlagHasTerminatingNullCharacter in the StringImpl m_hashAndFlags.

[Michael Saboff]

Committed r134981: <http://trac.webkit.org/changeset/134981>