Bug 35144 - [GTK][Qt] plugins/set-status.html introduced in r54993 fails
Summary: [GTK][Qt] plugins/set-status.html introduced in r54993 fails
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-02-19 03:52 PST by Philippe Normand
Modified: 2010-05-09 02:18 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.99 KB, patch)
2010-05-03 12:15 PDT, Robert Hogan
ap: review-
Details | Formatted Diff | Diff
Patch (3.07 KB, patch)
2010-05-05 12:59 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (2.82 KB, patch)
2010-05-05 13:01 PDT, Robert Hogan
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-02-19 03:52:43 PST
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
Comment 1 Andras Becsi 2010-02-19 06:41:19 PST
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
Comment 2 Alexey Proskuryakov 2010-02-19 09:06:18 PST
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.
Comment 3 Alexey Proskuryakov 2010-02-19 10:28:50 PST
Filed bug 35165 for the Windows failure. I think that Mac behavior should change here.
Comment 4 Tor Arne Vestbø 2010-03-10 06:41:59 PST
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
Comment 5 Robert Hogan 2010-05-03 12:15:40 PDT
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().
Comment 6 Alexey Proskuryakov 2010-05-03 12:29:30 PDT
The fix doesn't look right to me - the QString should not be encoded, it's supposed to be UTF-16 inside.
Comment 7 Robert Hogan 2010-05-04 13:48:56 PDT
(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?
Comment 8 Alexey Proskuryakov 2010-05-04 14:12:36 PDT
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.
Comment 9 Alexey Proskuryakov 2010-05-04 14:13:36 PDT
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.
Comment 10 Robert Hogan 2010-05-05 11:32:00 PDT
(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 'Ð'.
Comment 11 Alexey Proskuryakov 2010-05-05 11:41:34 PDT
> 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.
Comment 12 Robert Hogan 2010-05-05 12:26:47 PDT
(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!
Comment 13 Robert Hogan 2010-05-05 12:59:58 PDT
Created attachment 55145 [details]
Patch
Comment 14 Robert Hogan 2010-05-05 13:01:53 PDT
Created attachment 55146 [details]
Patch

Whoops.
Comment 15 Alexey Proskuryakov 2010-05-05 13:17:30 PDT
Comment on attachment 55146 [details]
Patch

r=me
Comment 16 Alexey Proskuryakov 2010-05-05 13:18:31 PDT
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.
Comment 17 Eric Seidel (no email) 2010-05-08 23:15:16 PDT
Attachment 55146 [details] was posted by a committer and has review+, assigning to Robert Hogan for commit.
Comment 18 Robert Hogan 2010-05-09 02:18:43 PDT
Landed in r59016.