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

[Adam Domurad]

On 11/30/2012 12:03 PM, Pavel Tisnovsky wrote:
> 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!

Admittedly I do think its a bit elegant :)

> 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.

I can do this in an update, although it will make the code slightly 
harder to understand (but may be worth the easy catching of off-by-one 
errors). As we discussed, running the test suite with a guarded malloc 
(address-sanitizer, valgrind, etc) is the fully correct solution. The 
majority of the allocations are not done through these browser 
functions, after all.

Another thought I just had (but I will leave it for an update), I can 
distinguish between never allocating memory with the mock function, and 
attempted double-frees (currently it will tell you that the memory was 
never allocated with the function for both cases).

>
> -------------
>
>          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!)

As we discussed this code is not actually a generic double conversion 
but a conversion for specially crafted strings passed from icedtea-web. 
As such the decimal point is always guaranteed. The code could be 
cleaned up to make this clearer, however I was trying to avoid creeping 
the scope of my refactoring.

>
> -------------
>
> Cheers,
> Pavel
>
>
Thanks for the review!

Happy hacking,
-Adam