[icedtea-web][rfc] Refactor a JS<->Java function, add browser-table mocking to C++ unit tests

[Pavel Tisnovsky]

Hi Adam,

I think that your patches are ok. I have just two notes:

-------------

+// It is expected that these will only run during a unit test
+static void* mock_memalloc(uint32_t size) {
+    void* mem = malloc(size);
+    __allocations.insert(mem);
+    return mem;
+}
+
+static void mock_memfree(void* ptr) {
+    if (__allocations.erase(ptr)) {
+        free(ptr);
+    } else {
+        printf("Attempt to free memory with browserfunctions.memfree that was not allocated by the browser!\n");
+        CHECK(false);
+    }
+}

This is tiny, very nice & useful solution for basic memory checks!
For the future version it would be good to allocate bigger memory block, say 16 bytes + size + 16 bytes, fill the
prefix(16B) and suffix(16B) with some pattern and return (malloc() + 16) (but whole block will be saved into __allocations, ie. size+32).

On memfree() you could easily check if both prefix and suffix blocks are not corrupted, which is a quick & dirty
overflow checking.

-------------

        double d = strtod(value.c_str(), NULL);

        // See if it is convertible to int
        if (value.find(".") != std::string::npos || d < -(0x7fffffffL - 1L) || d > 0x7fffffffL)
        {
            PLUGIN_DEBUG("Method call returned a double %f\n", d);
            DOUBLE_TO_NPVARIANT(d, variant);
        } else
        {
            int32_t i = (int32_t) d;
            PLUGIN_DEBUG("Method call returned an int %d\n", i);
            INT32_TO_NPVARIANT(i, variant);
        }

Well it's older code but still - I'm not sure whether we need to check for numbers like "1E-10" etc. too.
They can be converted to double but does not contain '.' in the string form
and are still inside the unsigned 32bit range...
AFAIK it should be compatible with Rhino implementation (I need to check it!)

-------------

Cheers,
Pavel


----- Original Message -----
> Hi all. This refactoring will help in the fix for the JSObject bug
> [1]
> 
> Firstly, to aid in testing this I added a simple mechanism for
> mocking
> browser callbacks:
> 
> Testing framework ChangeLog:
> 2012-11-XX  Adam Domurad  <adomurad at redhat.com>
> 
>      Added a simple mechanism for mocking functions in the browser
>      function
>      table. Can be exapanded as needed.
>      * tests/cpp-unit-tests/main.cc: Call setup function, warn on
>      browser
>      function based memory leak.
>      * tests/cpp-unit-tests/browser_mock.cc: New, implements simple
>      error-checking mocks of browser callbacks.
>      * tests/cpp-unit-tests/browser_mock.h: New, interface to mocking
>      functions.
> 
> A further idea for testing these functions (which rely on
> JavaRequestProcessor) is to have a different version of this class
> when
> compiling for unit tests. This would effectively 'mock' the class
> without having to modify the extensibility of the actual class (eg
> can
> make everything virtual but thats intrusive).
> 
> The refactoring introduces two convenience methods, and breaks up
> IcedTeaPluginUtilities::javaResultToNPVariant into multiple, more
> manageable parts. This will make it easier to apply the fix (which
> requires a special case for sun.plugin.JSObject like the one done on
> java.lang.String). This refactoring is in jsfunc-refactor.patch, and
> (since the patch doesn't read well) copied into
> newjsfunc-snippet.cpp.
> 
> Refactoring ChangeLog:
> 2012-11-XX  Adam Domurad  <adomurad at redhat.com>
> 
>      Breaks up IcedTeaPluginUtilities::javaResultToNPVariant into
>      multiple,
>      more manageable parts.
>      * plugin/icedteanp/IcedTeaPluginUtils.cc: Make three helper
>      functions
>      for the different cases. Two new helper functions for converting
>      from
>      std::string to NPString and NPVariant.
>      * plugin/icedteanp/IcedTeaPluginUtils.h: Two new helper
>      functions.
>      * tests/cpp-unit-tests/IcedTeaPluginUtilsTest.cc: Tests for the
>      new
>      NPString and NPVariant from std::string functions.
> 
> 
> [1] http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1198
> 
> 
> Happy hacking,
> -Adam
>