Bug 99977 – [WK2] Regression(r131990): plugins/npruntime/remove-property.html started failing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

plugins/npruntime/remove-property.html started failing consistently after r131990 so it looks like a regression.

I confirmed this on Mac, GTK and EFL ports.

The diff looks like:
--- /Volumes/Data/slave/mountainlion-debug-tests-wk2/build/layout-test-results/plugins/npruntime/remove-property-expected.txt
+++ /Volumes/Data/slave/mountainlion-debug-tests-wk2/build/layout-test-results/plugins/npruntime/remove-property-actual.txt
@@ -7,5 +7,5 @@
 PASS 'property' in obj is true
 PASS 'property' in obj is false
 PASS array[1] is 2
-PASS array[1] is undefined
+FAIL array[1] should be undefined (of type undefined). Was 2 (of type number).

[Chris Dumez]

Skipped for EFL-WK2 in r132064.

[Anders Carlsson]

Created attachment 169956 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 169956 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        a temporary to be created which the number was then decoded into.

Is there a way to prevent this from happening in the future?

[Anders Carlsson]

(In reply to comment #3)
> (From update of attachment 169956 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169956&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        a temporary to be created which the number was then decoded into.
> 
> Is there a way to prevent this from happening in the future?

Yeah, I'm going to remove the overload.

[Darin Adler]

Comment on attachment 169956 [details]
Patch

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

> Source/WebKit2/Shared/Plugins/NPIdentifierData.cpp:76
> -        encoder->encode(static_cast<int32_t>(m_number));
> +        encoder->encode(m_number);

Why are all the other encode casts still a good idea and this one not?

> Source/WebKit2/Shared/Plugins/NPIdentifierData.cpp:87
> -    return decoder->decode(static_cast<int32_t>(result.m_number));
> +    return decoder->decode(result.m_number);

This cast is clearly wrong, but can’t we create an explicit temporary of the right type, instead? Was the whole idea of using these type casts a flawed one?

Why didn’t the compiler give an error, since we were passing a temporary as a non-const reference argument? Is that a compiler bug?

[Anders Carlsson]

Committed r132120: <http://trac.webkit.org/changeset/132120>

[Anders Carlsson]

(In reply to comment #5)
> (From update of attachment 169956 [details])

> Why didn’t the compiler give an error, since we were passing a temporary as a non-const reference argument? Is that a compiler bug?

The ArgumentDecoder overload takes a const reference and casts away the constness. I'm going to get rid of that overload.

[Anders Carlsson]

(In reply to comment #5)
> (From update of attachment 169956 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169956&action=review
> 
> > Source/WebKit2/Shared/Plugins/NPIdentifierData.cpp:76
> > -        encoder->encode(static_cast<int32_t>(m_number));
> > +        encoder->encode(m_number);
> 
> Why are all the other encode casts still a good idea and this one not?
>

I think that the ones that cast between int32_t and int are completely unnecessary.