Bug 35144 – [GTK][Qt] plugins/set-status.html introduced in r54993 fails

• Status: CLOSED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r55007%20%283318%29/plugins/set-status-pretty-diff.html

Direct DRT output seems to be fine:

UI DELEGATE STATUS CALLBACK: setStatusText:PÐSS

But the expected.txt file is wrong nonetheless:

UI DELEGATE STATUS CALLBACK: setStatusText:PÃÂSS

[Andras Becsi]

This test fails on the Qt bot and the Windows bot seems to have the problem too. The Qt port dumps the UI DELEGATE STATUS CALLBACK: setStatusText: twice and there seems to be an encoding problem too.

http://build.webkit.org/results/Qt%20Linux%20Release/r55012%20%287584%29/plugins/set-status-pretty-diff.html

http://build.webkit.org/results/Windows%20Release%20%28Tests%29/r55012%20%289239%29/plugins/set-status-pretty-diff.html

[Alexey Proskuryakov]

Trying to set status for null message may or may not be a bug - I've just followed what Mac in-process code did. The Unicode differences on Qt/Gtk are clearly a bug.

[Alexey Proskuryakov]

Filed bug 35165 for the Windows failure. I think that Mac behavior should change here.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords

[Robert Hogan]

Created attachment 54951 [details]
Patch

This fixes it - the QString is already UTF-8 encoded so it was converting \xD090 to \xC390\xC290. \C390 is the D with a pair of horizontal bars through it.

I'm not sure if this is to be expected or if I'm just working around a bug in QString::toUtf8().

[Alexey Proskuryakov]

The fix doesn't look right to me - the QString should not be encoded, it's supposed to be UTF-16 inside.

[Robert Hogan]

(In reply to comment #6)
> The fix doesn't look right to me - the QString should not be encoded, it's
> supposed to be UTF-16 inside.

I'm probably way off, but is the problem in JavasciptCore/wtf/qt/StringQt.cpp then? Should it detect if the String it is casting is already UTF-8 encoded, as in the case of this test?

[Alexey Proskuryakov]

String code shouldn't look inside strings passed to constructor to determine their encoding.

I don't know where exactly the bug is, but some code clearly has a wrong assumption about buffer content, and uses a wrong encoding to construct String or QString. This can be in NPAPI or in DumpRenderTree bindings.

[Alexey Proskuryakov]

Comment on attachment 54951 [details]
Patch

Marking r- to get this out of review queue. I hoped that a reviewer more knowledgeable about Qt would chime in, but this hasn't happened yet.

[Robert Hogan]

(In reply to comment #6)
> The fix doesn't look right to me - the QString should not be encoded, it's
> supposed to be UTF-16 inside.

This is the call chain as I understand it.

The string passed from the test plugin does appear to be UTF-8:

static bool testSetStatus(PluginObject* obj, const NPVariant* args, uint32_t argCount, NPVariant* result)
{
    char* message = 0;
    if (argCount && NPVARIANT_IS_STRING(args[0])) {
        NPString statusString = NPVARIANT_TO_STRING(args[0]);
        message = toCString(statusString);
    
    browser->status(obj->npp, message);
}

static char* toCString(const NPString& string)
{
    char* result = static_cast<char*>(malloc(string.UTF8Length + 1));
    memcpy(result, string.UTF8Characters, string.UTF8Length);
    result[string.UTF8Length] = '\0';

    return result;
}

PluginView casts it to a String:

void PluginView::status(const char* message)
{
    if (Page* page = m_parentFrame->page())
        page->chrome()->setStatusbarText(m_parentFrame.get(), String(message));
}

As far as I can tell, displayStringModifiedByEncoding() isn't doing anything noteworthy to the string, unless it contains backslashes. 

void Chrome::setStatusbarText(Frame* frame, const String& status)
{
    ASSERT(frame);
    m_client->setStatusbarText(frame->displayStringModifiedByEncoding(status));
}

ChromeClientQt casts it to a QString:

void ChromeClientQt::setStatusbarText(const String& msg)
{
    QString x = msg;
    emit m_webPage->statusBarMessage(msg);
}

So there's no conversion to UTF-16 along the way here unless that it is implicit in one of the casts. That leads me to think that doing :


    printf("UI DELEGATE STATUS CALLBACK: setStatusText:%s\n", message.toUtf8().constData());

in DumpRenderTreeQt is converting UTF-8 to UTF-8 a second time, which is why we get 'ÃÂ', which as far as I can tell is an attempt to UTF-8 encode 'Ð'.

[Alexey Proskuryakov]

> void PluginView::status(const char* message)
> {
>   if (Page* page = m_parentFrame->page())
>       page->chrome()->setStatusbarText(m_parentFrame.get(), String(message));
>}

This is wrong, and should use String::fromUTF8(). The String constructor treats its char* input as latin-1.

[Robert Hogan]

(In reply to comment #11)
> > void PluginView::status(const char* message)
> > {
> >   if (Page* page = m_parentFrame->page())
> >       page->chrome()->setStatusbarText(m_parentFrame.get(), String(message));
> >}
> 
> This is wrong, and should use String::fromUTF8(). The String constructor treats
> its char* input as latin-1.

Excellent, thanks Alexey!

[Robert Hogan]

Created attachment 55145 [details]
Patch

[Robert Hogan]

Created attachment 55146 [details]
Patch

Whoops.

[Alexey Proskuryakov]

Comment on attachment 55146 [details]
Patch

r=me

[Alexey Proskuryakov]

You might want to test what happens if a plug-in passes invalid UTF-8. String::fromUTF8() will make a null string, which may or may not be a problem downstream.

[Eric Seidel (no email)]

Attachment 55146 [details] was posted by a committer and has review+, assigning to Robert Hogan for commit.

[Robert Hogan]

Landed in r59016.