811057 - $OBJDIR/dom/bindings/MediaStreamListBinding.cpp:77:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

{
$OBJDIR/dom/bindings/MediaStreamListBinding.cpp: In function ‘bool mozilla::dom::MediaStreamListBinding::DeferredFinalize(int32_t, void*)’:

$OBJDIR//dom/bindings/MediaStreamListBinding.cpp:77:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
}

For this chunk of Codegen.py:
> 804 class CGDeferredFinalize(CGAbstractStaticMethod):
> 805     def __init__(self, descriptor):
> 806         CGAbstractStaticMethod.__init__(self, descriptor, "DeferredFinalize", "bool", [Argument('int32_t', 'slice'), Argument('void*', 'data')])
> 807 
> 808     def definition_body(self):
> 809         smartPtr = DeferredFinalizeSmartPtr(self.descriptor)
> 810         return """  nsTArray<%(smartPtr)s >* pointers = static_cast<nsTArray<%(smartPtr)s >*>(data);
> 811   uint32_t oldLen = pointers->Length();
> 812   if (slice == -1 || slice > oldLen) {
> 813     slice = oldLen;
> 814   }
> 815   uint32_t newLen = oldLen - slice;
> 816   pointers->RemoveElementsAt(newLen, slice);

https://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#804

"slice" is signed, "oldLen" is unsigned.  It looks like we're basically treating slice as if it were unsigned, though, aside from the "-1" comparison.

Perhaps we should be asserting that it's either -1 or nonnegative? (and then cast it to be unsigned in the comparison to |oldLen|?)

And/or perhaps we should just have an "unsignedSlice" variable here which we set at line 813 and use in place of slice for the rest of the method?

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> It looks like we're basically
> treating slice as if it were unsigned, though, aside from the "-1"
> comparison.

Yeah, https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.h#733 confirms that:
> 733     // Called to finalize a number of objects. Slice is the number of objects to
> 734     // finalize, if it's -1 all objects should be finalized. data is the pointer

"number of objects to finalize" sounds nonnegative (aside from the -1 special value)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This fixes the build warning for me.

Comment 3

[Peter Van der Beken [:peterv]]

How about making DeferredFinalizeFunction take a uint32_t and passing in UINT32_MAX instead of -1? The check could then just be |if (slice > oldLen)|.

Comment 4

[Peter Van der Beken [:peterv]]

Or we could just do slice = NS_MIN(slice, oldLen)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2

OK. That's a slightly more invasive change, but it makes sense.

This takes care of it, I think. (combining comment 3 & comment 4)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 682125 [details] [diff] [review]
fix v2

>diff --git a/js/xpconnect/src/XPCJSRuntime.cpp b/js/xpconnect/src/XPCJSRuntime.cpp
>-    int32_t counter = 0;
>+    uint32_t counter = 0;
[...]
>-        if (slice > 0 && ++counter == slice) {
>+        if (slice != UINT32_MAX && ++counter == slice) {
>             return items->IsEmpty();
>         }

NOTE: This might look like it changes behavior in the situation where slice = 0 -- previously, we would've failed the first condition there ( > 0), whereas now we'll pass ( != UINT32_MAX).

However, it'll still be guaranteed to fail the second condition, since |++counter| is guaranteed to evaluate to something nonzero (disregarding cases where counter wraps around from +infinity).  So we'll still have the same behavior there -- we'll fail the "if" check.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v2a

(Missed one chunk in previous patch version -- s/-1/UINT32_MAX/ on a place where these DeferredFinalizeFunctions are invoked.)

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 682133 [details] [diff] [review]
fix v2a

Review of attachment 682133 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +547,5 @@
>          nsISupports *wrapper = items->ElementAt(count - 1);
>          items->RemoveElementAt(count - 1);
>          NS_RELEASE(wrapper);
>  
> +        if (slice != UINT32_MAX && ++counter == slice) {

Do we still need |slice != UINT32_MAX|? If slice is UINT32_MAX and counter == slice we've dealt with all items anyway, so returning items->IsEmpty() would be the same as returning true (which we'd do currently). Unless I'm missing something.
Also, it seems nonsensical to have slice == 0, so maybe we should MOZ_ASSERT that at the beginning?

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v3

(In reply to Peter Van der Beken [:peterv] from comment #8)
> Do we still need |slice != UINT32_MAX|?

We don't -- you're right.

I actually restructured that chunk of code to use a for-loop, up to slice (which I set to NS_MIN(slice, oldLen) along the lines of comment 4).  This removes the need for the special early-return & "break" statement, making the control flow clearer IMHO.

> Also, it seems nonsensical to have slice == 0, so maybe we should MOZ_ASSERT
> that at the beginning?

It looks like it, yeah.  Added a MOZ_ASSERT to the generated function and the hardcoded "ReleaseSliceNow" impl in XPCJSRuntime.

Two other minor changes:
 (a) Tweaked the documentation in xpcprivate.h to clarify the the meaning of the return value.
 (b) I caught one straggling s/-1/UINT32_MAX/, in GCCallback().  (That's the last one -- I did an audit for other mentions of "run" and "mDeferredFinalizeFunctions" in the file to be sure there weren't any other ones that I'd missed.)

Requesting one more review, since I reworked the logic in ReleaseSliceNow().

Comment 10

[Daniel Holbert [:dholbert]]

Green try run (modulo a few intermittent oranges):
 https://tbpl.mozilla.org/?tree=Try&rev=5737da7bca87

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 683838 [details] [diff] [review]
fix v3

Review of attachment 683838 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks!

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +539,5 @@
>      nsTArray<nsISupports *>* items =
>          static_cast<nsTArray<nsISupports *>*>(data);
> +
> +    slice = NS_MIN(slice, items->Length());
> +    for (uint32_t i = 0; i < slice; i++) {

++i

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Peter Van der Beken [:peterv] from comment #11)
> > +    for (uint32_t i = 0; i < slice; i++) {
> 
> ++i

Fixed, and pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc2a8f21aea

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6cc2a8f21aea