717004 - nsGenericHTMLElement.cpp:1627:12: error: variable ‘rv’ set but not used [-Werror=unused-but-set-variable]

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Bug 716338 turned on warnings-as-errors in content/html/content/src/Makefile.in

This breaks my local build (with --enable-warnings-as-errors), due to this build warning:
{
  nsGenericHTMLElement.cpp:1627:12: error: variable ‘rv’ set but not used [-Werror=unused-but-set-variable]
}
which points to these line of code:
> 1627   nsresult rv = GetLayoutHistoryAndKey(aContent, true,
> 1628                                        getter_AddRefs(history), key);
[...]
> 1635   rv = history->GetState(key, &state);
where we capture GetLayoutHistoryAndKey's return value and ignore it.

That issue actually dates back to a patch for bug 108309 that landed in 2002:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/html/content/src&command=DIFF_FRAMESET&file=nsGenericHTMLElement.cpp&rev2=1.343&rev1=1.342

That patch added one other call to GetLayoutHistoryAndKey, whose return-value is *not* checked (and still isn't checked to this day).  Given that we've always ignored the return-value in both places (explicitly in one place, implicitly in the other), seems like we might as well make both places explicitly ignore it.

Comment 1

[Daniel Holbert [:dholbert]]

FWIW, the only way that GetLayoutHistoryAndKey can fail is if nsContentUtils::GenerateStateKey fails.

GenerateStateKey, in turn, has "return NS_OK;" as its only explicit return statements.  It also has 2 NS_ENSURE_ statements that could return failure codes, if (a) a null 'aContent' pointer is passed in, or (b) if we're out of memory.

We know the former isn't true, because we dereference the passed-in aContent pointer in GetLayoutHistoryAndKey before we call GenerateStateKey.

And if the latter is true, we're kind of screwed anyway.

So I think it's safe to just drop the "nsresult rv =" and assume that GetLayoutHistoryAndKey succeeds (as we already do elsewhere).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 3

[Mounir Lamouri (:mounir)]

Comment on attachment 587448 [details] [diff] [review]
fix

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

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +1631,5 @@
>    }
>  
>    nsPresState *state;
>    // Get the pres state for this key
> +  nsresult rv = history->GetState(key, &state);

You don't seem to be using |rv| again... Seems like you could just remove |rv| from the patch.
BTW, I would change:
if (state) {
  // blah
}

return false;
to:
if (!state) {
  return false;
}

// blah

Comment 4

[Daniel Holbert [:dholbert]]

Oh -- I didn't notice it, until now, but that part is addressed in bug 717015. :)  (with an rv check)

I'll post a merged patch here.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Here's the patch with bug 717015's fix merged in.

So when this "GetState()" call originally landed, it was passing in a nsCOMPtr, which was guaranteed to be left at its default value (null) if GetState failed.  So the null-check was basically sufficient/correct at that time.

However, now we're passing in a raw uninitialized pointer, and if GetState were to fail, we absolutely should not be relying on that raw pointer being null or containing anything useful.

So, this patch adds a NS_SUCCEEDED() check before we test that pointer's value.

(As it turns out, we actually know that the underlying impl for this interface method will always succeed; still, best not to hardcode that assumption in if we don't have to.)

Comment 7

[Mounir Lamouri (:mounir)]

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

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

r+ with the follow-up opened.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +1624,5 @@
>  {
>    nsCOMPtr<nsILayoutHistoryState> history;
>    nsCAutoString key;
> +  GetLayoutHistoryAndKey(aContent, true,
> +                         getter_AddRefs(history), key);

Could you open a follow-up to make GetLayoutHistoryAndKey infallible? Seems like the only reason to not return NS_OK is when GenerateStateKey doesn't return NS_OK but it happens that this method *always* return NS_OK. Let's remove the |nsresult| return value.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #7)
> Seems
> like the only reason to not return NS_OK is when GenerateStateKey doesn't
> return NS_OK but it happens that this method *always* return NS_OK. Let's
> remove the |nsresult| return value.

Not quite -- GenerateStateKey can fail. (via NS_ENSURE_* statments)  See comment 1.

Comment 9

[Daniel Holbert [:dholbert]]

I'm still happy to file the followup, though; GetLayoutHistoryAndKey could still change to indicate failure by returning a null |history| pointer. (which is basically what we depend on right now anyway.)

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Not quite -- GenerateStateKey can fail. (via NS_ENSURE_* statments)  See
> comment 1.

Oups... Sorry :(

(In reply to Daniel Holbert [:dholbert] from comment #9)
> I'm still happy to file the followup, though; GetLayoutHistoryAndKey could
> still change to indicate failure by returning a null |history| pointer.
> (which is basically what we depend on right now anyway.)

Seems like |GetLayoutHistoryAndKey| returns NS_OK but has a null |history| pointer in one case. Maybe some callers are using that?

Comment 11

[Daniel Holbert [:dholbert]]

Filed bug 717066 as the followup requested in comment 7.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #10)
> Seems like |GetLayoutHistoryAndKey| returns NS_OK but has a null |history|
> pointer in one case. Maybe some callers are using that?

I don't think so.  In any case, we can figure that out in the followup. (bug 717066)

Comment 12

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b40b3847195

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5b40b3847195