Bug 113139 – Call Netscape Plugin's toString() and valueOf() instead of providing default implementation

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Arunprasad]

Calling a toString()/valueOf() method on NPObject returns some weird value like "NPObject 0x800000, NPClass 0x810000".

For example,

var obj = document.getElementById("pluginElement");
var npObject = obj.getApplication(); //getApplication is a native method implemented in a plugin and return NPObject
console.log("name of the npObject"+npObject); // which prints something like "NPObject 0x800000, NPClass 0x810000"


Can we delegate this call to npapi plugin, instead of providing default value? I saw that this have been done in c_instance.cpp,

JSValue CInstance::stringValue(ExecState* exec) const
{
    char buf[1024];
    snprintf(buf, sizeof(buf), "NPObject %p, NPClass %p", _object, _object->_class);
    return jsString(exec, buf);
}

So instead of providing default implementation, let this to call "toString()"/"valueOf()" method of plugin, if it doesn't exists the let it to fallback to WebCore's default implementation.

JSValue CInstance::stringValue(ExecState* exec)
{
    NPIdentifier ident = NPN_GetStringIdentifier("toString");
    if (!_object->_class->hasMethod(_object, ident))
        return jsUndefined();

    // Invoke the 'C' method.
    bool retval = true;
    NPVariant resultVariant;
    VOID_TO_NPVARIANT(resultVariant);

    {
        JSLock::DropAllLocks dropAllLocks(exec);
        ASSERT(globalExceptionString().isNull());
        retval = _object->_class->invoke(_object, ident, 0, 0, &resultVariant);
        moveGlobalExceptionToExecState(exec);
    }
    
    if (!retval)
        throwError(exec, createError(exec, "Error calling method on NPObject."));

    JSValue resultValue = convertNPVariantToValue(exec, &resultVariant, m_rootObject.get());
    _NPN_ReleaseVariantValue(&resultVariant);
    return resultValue;
}

I would like to provide a patch which looks something similar like above to solve this issue. Is it acceptable?

[Eric Seidel (no email)]

I'm not actively workign on plugins.  I'm not sure who is.

[Arunprasad]

(In reply to comment #1)
> I'm not actively workign on plugins.  I'm not sure who is.

Thanks Eric, Will try in IRC :)

[Arunprasad Rajkumar]

Created attachment 194714 [details]
Patch

[Anders Carlsson]

Comment on attachment 194714 [details]
Patch

Patch looks good but needs a layout test.

[Arunprasad]

(In reply to comment #4)
> (From update of attachment 194714 [details])
> Patch looks good but needs a layout test.

Thanks for your valuable feedback Anders :) I will look into webkit/LayoutTests/plugins/npruntime inorder to understand how the existing test was done.

[Arunprasad Rajkumar]

Created attachment 194742 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/plugins/npruntime/tostring-expected.txt', u'LayoutTests/plugins/npruntime/tostring.html', u'LayoutTests/plugins/npruntime/valueof-expected.txt', u'LayoutTests/plugins/npruntime/valueof.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/bridge/c/c_instance.cpp', u'Source/WebCore/bridge/c/c_instance.h', u'Tools/ChangeLog', u'Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp']" exit_code: 1
Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:100:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 9 files


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

[Arunprasad]

(In reply to comment #7)
> Attachment 194742 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/plugins/npruntime/tostring-expected.txt', u'LayoutTests/plugins/npruntime/tostring.html', u'LayoutTests/plugins/npruntime/valueof-expected.txt', u'LayoutTests/plugins/npruntime/valueof.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/bridge/c/c_instance.cpp', u'Source/WebCore/bridge/c/c_instance.h', u'Tools/ChangeLog', u'Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp']" exit_code: 1
> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:100:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Total errors found: 4 in 9 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Oops :(

I just followed the existing style which was used for Plugin Properties, 

enum {
    ID_PROPERTY_FOO = 0,
    ID_PROPERTY_BAR,
    ID_PROPERTY_OBJECT_POINTER,
    ID_PROPERTY_TEST_OBJECT,
    ID_PROPERTY_REF_COUNT,
    NUM_TEST_IDENTIFIERS,
};

Is it a false-positive ? (or) Properties follows very old guidelines? Shall I change all properties & method as indicated by style-checker( enum members should use InterCaps with an initial capital letter.)?

[Build Bot]

Comment on attachment 194742 [details]
Patch

Attachment 194742 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17310044

[WebKit Review Bot]

Comment on attachment 194742 [details]
Patch

Attachment 194742 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17188842

New failing tests:
fast/css/font-family-pictograph.html

[WebKit Review Bot]

Created attachment 194743 [details]
Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Arunprasad Rajkumar]

Created attachment 194744 [details]
Patch

[Build Bot]

Comment on attachment 194744 [details]
Patch

Attachment 194744 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17293262

[Build Bot]

Comment on attachment 194744 [details]
Patch

Attachment 194744 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17120830

[Arunprasad Rajkumar]

Created attachment 194750 [details]
Patch

[Arunprasad Rajkumar]

Created attachment 194753 [details]
Patch

[Build Bot]

Comment on attachment 194753 [details]
Patch

Attachment 194753 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17228529

New failing tests:
plugins/npruntime/tostring.html
plugins/npruntime/valueof.html

[Build Bot]

Created attachment 194761 [details]
Archive of layout-test-results from webkit-ews-05 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-future  Platform: Mac OS X 10.8.2

[WebKit Review Bot]

Comment on attachment 194753 [details]
Patch

Attachment 194753 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17317025

New failing tests:
fast/css/font-family-pictograph.html

[WebKit Review Bot]

Created attachment 194762 [details]
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Arunprasad]

(In reply to comment #17)
> (From update of attachment 194753 [details])
> Attachment 194753 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17228529
> 
> New failing tests:
> plugins/npruntime/tostring.html
> plugins/npruntime/valueof.html

I don't know how it is failing only for cr-linux(release). But why not cr-linux-debug?

[Arunprasad]

(In reply to comment #17)
> (From update of attachment 194753 [details])
> Attachment 194753 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17228529
> 
> New failing tests:
> plugins/npruntime/tostring.html
> plugins/npruntime/valueof.html

(In reply to comment #21)
> (In reply to comment #17)
> > (From update of attachment 194753 [details] [details])
> > Attachment 194753 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-commit-queue.appspot.com/results/17228529
> > 
> > New failing tests:
> > plugins/npruntime/tostring.html
> > plugins/npruntime/valueof.html
> 
> I don't know how it is failing only for cr-linux(release). But why not cr-linux-debug?

I guess Chromium port never use NPAPI plugin, so they completely avoided LayoutTests/plugins/*, but they are running any new tests if added to LayoutTests. Can anybody from Chromium Team help me to understand this ? Thanks in advance :)

[Vivek Galatage]

(In reply to comment #21)
> (In reply to comment #17)
> > (From update of attachment 194753 [details] [details])
> > Attachment 194753 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-commit-queue.appspot.com/results/17228529
> > 
> > New failing tests:
> > plugins/npruntime/tostring.html
> > plugins/npruntime/valueof.html
> 
> I don't know how it is failing only for cr-linux(release). But why not cr-linux-debug?

Because only cr-linux-release runs the tests while cr-linux-debug doen't. Having said that its interesting to find out how the tests are passing on mac-wk2 (I suppose this bot also runs the tests). Also check the test expectation file for the mac to check whether NPAPI tests are being skipped there.

[Arunprasad Rajkumar]

Created attachment 194908 [details]
Patch

[Arunprasad]

(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #17)
> > > (From update of attachment 194753 [details] [details] [details])
> > > Attachment 194753 [details] [details] [details] did not pass mac-ews (mac):
> > > Output: http://webkit-commit-queue.appspot.com/results/17228529
> > > 
> > > New failing tests:
> > > plugins/npruntime/tostring.html
> > > plugins/npruntime/valueof.html
> > 
> > I don't know how it is failing only for cr-linux(release). But why not cr-linux-debug?
> 
> Because only cr-linux-release runs the tests while cr-linux-debug doen't. Having said that its interesting to find out how the tests are passing on mac-wk2 (I suppose this bot also runs the tests). Also check the test expectation file for the mac to check whether NPAPI tests are being skipped there.

Thanks for your clue Vivek :) only qt-mac completely skips the npapi related tests. Other ports omits very few which are not relavent to this bug's testcases. I'm trying out to run LayoutTests for chromium linux build.

[Build Bot]

Comment on attachment 194908 [details]
Patch

Attachment 194908 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17144832

New failing tests:
plugins/npruntime/tostring.html
plugins/npruntime/valueof.html

[Build Bot]

Created attachment 194945 [details]
Archive of layout-test-results from webkit-ews-04 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-future  Platform: Mac OS X 10.8.2

[Arunprasad Rajkumar]

Created attachment 196188 [details]
Patch

[Benjamin Poulain]

Comment on attachment 196188 [details]
Patch

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

r- because of the tests.

They are not bad, but they could be a lot better.

Please get Anders for a full review.

> Source/WebCore/bridge/c/c_instance.cpp:284
> +    // Fallback to default implementation

Missing period at the end of the sentence.

> Source/WebCore/bridge/c/c_instance.cpp:298
> +    // ECMA 9.2

This comment is too terse.

> Source/WebCore/bridge/c/c_instance.cpp:308
> +    // Fallback to default implementation

Missing period.

> Source/WebCore/bridge/c/c_instance.cpp:312
> +bool CInstance::convertToPrimitive(ExecState* exec, const char* name, JSValue& resultValue) const

Maybe it makes sense in NPAPI, but for me, convertToPrimitive() is a mysterious name to me.
Could this be named better?

> Source/WebCore/bridge/c/c_instance.cpp:318
> +    // Invoke the 'C' method.

'C'? :)

> Source/WebCore/bridge/c/c_instance.cpp:331
> +        throwError(exec, createError(exec, "Error calling method on NPObject."));

->ASCIILiteral("Error calling method on NPObject.")

> Source/WebCore/bridge/c/c_instance.h:75
>      JSValue booleanValue() const;
> +    // Helper method to call NPObject's invoke on behalf of toString and valueOf.
> +    bool convertToPrimitive(ExecState*, const char*, JSValue&) const;

You should look for the best name possible instead of having a comment.

This should be private.

If the function is incorrect if not called from toString() or valueOf(), it should likely be a static function instead of an object method. If the call site does not matter, you should not list call sites in a comment because the comment will get out of date.

> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:154
> +        // Return classname as string

Missing period.

> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:158
> +        char* className = static_cast<char*>(browser->memalloc(11));
> +        strcpy(className, "TestObject");
> +        STRINGZ_TO_NPVARIANT(className, *result);
> +        return true;

No need to release the memory?

> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:161
> +        // Return some number

Period.

> LayoutTests/plugins/npruntime/tostring.html:12
> +        successCount ++;

Remove the extra space.

> LayoutTests/plugins/npruntime/tostring.html:15
> +    // Normal plugin.testObject + "" will call valueOf,
> +    // Do some tricks to make a call to implicit toString.

Do -> do.

> LayoutTests/plugins/npruntime/tostring.html:17
> +        successCount ++;

Remote the extra space.

> LayoutTests/plugins/npruntime/tostring.html:20
> +    if (successCount == 2)
> +        document.getElementById('result').innerHTML = 'SUCCESS';

Instead of that, the whole test should use the JavaScript test scripts.

The comparison should be done with shouldBeEqualToString(foo, bar);

The reason is on fail, one would know exactly which part failed. Currently, any failure would just mean there is no "SUCCESS" in the results. This is okay when it succeed, but it does not provide enough readability when it fails.

> LayoutTests/plugins/npruntime/tostring.html:25
> +This tests that toString() works correctly on NPObject.

Then with JavaScript test, you should use description("This tests that toString() works correctly on NPObject.").

> LayoutTests/plugins/npruntime/valueof.html:18
> +
> +    var successCount = 0;
> +    var plugin = document.getElementById("testPlugin");
> +
> +    if (plugin.testObject.valueOf() === 123456789)
> +        successCount ++;
> +
> +    if (plugin.testObject == 123456789)
> +        successCount ++;
> +
> +    if (successCount == 2)
> +        document.getElementById('result').innerHTML = 'SUCCESS';

Ditto.

[Arunprasad Rajkumar]

Comment on attachment 196188 [details]
Patch

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

>> Source/WebCore/bridge/c/c_instance.cpp:284
>> +    // Fallback to default implementation
> 
> Missing period at the end of the sentence.

It is my mistake, anyhow I will raise a bug against check-webkit-style, also will try to add this constraint to trap in EWS itself :)

>> Source/WebCore/bridge/c/c_instance.cpp:312
>> +bool CInstance::convertToPrimitive(ExecState* exec, const char* name, JSValue& resultValue) const
> 
> Maybe it makes sense in NPAPI, but for me, convertToPrimitive() is a mysterious name to me.
> Could this be named better?

Sure :) Will find one !!

>> Source/WebCore/bridge/c/c_instance.cpp:318
>> +    // Invoke the 'C' method.
> 
> 'C'? :)

NPAPI method is called as 'C' method

>> Source/WebCore/bridge/c/c_instance.cpp:331
>> +        throwError(exec, createError(exec, "Error calling method on NPObject."));
> 
> ->ASCIILiteral("Error calling method on NPObject.")

Sorry :( I was not aware of that class. Will change that :), AFAIK ASCIILiteral is to reduce the memory usage in case of const string, isn't it?

>> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:158
>> +        return true;
> 
> No need to release the memory?

Memory will be release by the JSValue CInstance::invokeMethod after converting NPAPI's NPVariant type to JSC's JSValue using _NPN_ReleaseVariantValue(&resultVariant);

>> LayoutTests/plugins/npruntime/tostring.html:20
>> +        document.getElementById('result').innerHTML = 'SUCCESS';
> 
> Instead of that, the whole test should use the JavaScript test scripts.
> 
> The comparison should be done with shouldBeEqualToString(foo, bar);
> 
> The reason is on fail, one would know exactly which part failed. Currently, any failure would just mean there is no "SUCCESS" in the results. This is okay when it succeed, but it does not provide enough readability when it fails.

I just blindly followed the existing test scripts like "LayoutTests/plugins/npruntime/construct.html". Your comment is really valid, will change accordingly :)

[Arunprasad Rajkumar]

Created attachment 196879 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #29)
> (From update of attachment 196188 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196188&action=review
> 
> r- because of the tests.
> 
> They are not bad, but they could be a lot better.
> 
> Please get Anders for a full review.
> 
> > Source/WebCore/bridge/c/c_instance.cpp:284
> > +    // Fallback to default implementation
> 
> Missing period at the end of the sentence.
> 
> > Source/WebCore/bridge/c/c_instance.cpp:298
> > +    // ECMA 9.2
> 
> This comment is too terse.
> 
> > Source/WebCore/bridge/c/c_instance.cpp:308
> > +    // Fallback to default implementation
> 
> Missing period.
> 
> > Source/WebCore/bridge/c/c_instance.cpp:312
> > +bool CInstance::convertToPrimitive(ExecState* exec, const char* name, JSValue& resultValue) const
> 
> Maybe it makes sense in NPAPI, but for me, convertToPrimitive() is a mysterious name to me.
> Could this be named better?
> 
> > Source/WebCore/bridge/c/c_instance.cpp:318
> > +    // Invoke the 'C' method.
> 
> 'C'? :)
> 
> > Source/WebCore/bridge/c/c_instance.cpp:331
> > +        throwError(exec, createError(exec, "Error calling method on NPObject."));
> 
> ->ASCIILiteral("Error calling method on NPObject.")
> 
> > Source/WebCore/bridge/c/c_instance.h:75
> >      JSValue booleanValue() const;
> > +    // Helper method to call NPObject's invoke on behalf of toString and valueOf.
> > +    bool convertToPrimitive(ExecState*, const char*, JSValue&) const;
> 
> You should look for the best name possible instead of having a comment.
> 
> This should be private.
> 
> If the function is incorrect if not called from toString() or valueOf(), it should likely be a static function instead of an object method. If the call site does not matter, you should not list call sites in a comment because the comment will get out of date.
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:154
> > +        // Return classname as string
> 
> Missing period.
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:158
> > +        char* className = static_cast<char*>(browser->memalloc(11));
> > +        strcpy(className, "TestObject");
> > +        STRINGZ_TO_NPVARIANT(className, *result);
> > +        return true;
> 
> No need to release the memory?
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:161
> > +        // Return some number
> 
> Period.
> 
> > LayoutTests/plugins/npruntime/tostring.html:12
> > +        successCount ++;
> 
> Remove the extra space.
> 
> > LayoutTests/plugins/npruntime/tostring.html:15
> > +    // Normal plugin.testObject + "" will call valueOf,
> > +    // Do some tricks to make a call to implicit toString.
> 
> Do -> do.
> 
> > LayoutTests/plugins/npruntime/tostring.html:17
> > +        successCount ++;
> 
> Remote the extra space.
> 
> > LayoutTests/plugins/npruntime/tostring.html:20
> > +    if (successCount == 2)
> > +        document.getElementById('result').innerHTML = 'SUCCESS';
> 
> Instead of that, the whole test should use the JavaScript test scripts.
> 
> The comparison should be done with shouldBeEqualToString(foo, bar);
> 
> The reason is on fail, one would know exactly which part failed. Currently, any failure would just mean there is no "SUCCESS" in the results. This is okay when it succeed, but it does not provide enough readability when it fails.
> 
> > LayoutTests/plugins/npruntime/tostring.html:25
> > +This tests that toString() works correctly on NPObject.
> 
> Then with JavaScript test, you should use description("This tests that toString() works correctly on NPObject.").
> 
> > LayoutTests/plugins/npruntime/valueof.html:18
> > +
> > +    var successCount = 0;
> > +    var plugin = document.getElementById("testPlugin");
> > +
> > +    if (plugin.testObject.valueOf() === 123456789)
> > +        successCount ++;
> > +
> > +    if (plugin.testObject == 123456789)
> > +        successCount ++;
> > +
> > +    if (successCount == 2)
> > +        document.getElementById('result').innerHTML = 'SUCCESS';
> 
> Ditto.

Updated a new patch which adheres to Benjamin's suggestions :)

[Anders Carlsson]

Comment on attachment 196879 [details]
Patch

The code change looks good, but for the test please make a specific plug-in test (See the Tests/ subdirectory in TestNetscapePlugIn).

[Arunprasad Rajkumar]

Created attachment 196946 [details]
Patch

[Arunprasad Rajkumar]

Created attachment 196950 [details]
Patch

[Anders Carlsson]

Comment on attachment 196950 [details]
Patch

r=me. I’m going to land this manually so I can add the test file to the Xcode project.

[Build Bot]

Comment on attachment 196950 [details]
Patch

Attachment 196950 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17502380

New failing tests:
plugins/npruntime/tostring.html
plugins/npruntime/valueof.html

[Build Bot]

Created attachment 196959 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Anders Carlsson]

Committed r147956: <http://trac.webkit.org/changeset/147956>