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

RESOLVED FIXED in mozilla12

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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).
(Assignee)

Updated

6 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
Created attachment 587448 [details] [diff] [review]
fix
Attachment #587448 - Flags: review?(bugs)
(Assignee)

Updated

6 years ago
Component: DOM → DOM: Core & HTML
(Assignee)

Updated

6 years ago
Blocks: 108309
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
Attachment #587448 - Flags: review?(bugs) → review-
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
Created attachment 587491 [details] [diff] [review]
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.)
(Assignee)

Updated

6 years ago
Attachment #587491 - Flags: review?(mounir)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 717015
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.
Attachment #587491 - Flags: review?(mounir) → review+
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
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.)
(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?
(Assignee)

Comment 11

6 years ago
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)
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b40b3847195
Hardware: x86_64 → All
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/5b40b3847195
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.