Last Comment Bug 717004 - nsGenericHTMLElement.cpp:1627:12: error: variable ‘rv’ set but not used [-Werror=unused-but-set-variable]
: nsGenericHTMLElement.cpp:1627:12: error: variable ‘rv’ set but not used [-Wer...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla12
Assigned To: Daniel Holbert [:dholbert]
:
:
Mentors:
: 717015 (view as bug list)
Depends on:
Blocks: buildwarning 108309
  Show dependency treegraph
 
Reported: 2012-01-10 13:05 PST by Daniel Holbert [:dholbert]
Modified: 2012-01-11 18:20 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.18 KB, patch)
2012-01-10 13:20 PST, Daniel Holbert [:dholbert]
mounir: review-
Details | Diff | Splinter Review
fix v2 (1.40 KB, patch)
2012-01-10 14:58 PST, Daniel Holbert [:dholbert]
mounir: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-01-10 13:05:53 PST
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] 2012-01-10 13:11:54 PST
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] 2012-01-10 13:20:07 PST
Created attachment 587448 [details] [diff] [review]
fix
Comment 3 Mounir Lamouri (:mounir) 2012-01-10 14:45:30 PST
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] 2012-01-10 14:52:39 PST
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] 2012-01-10 14:58:31 PST
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.)
Comment 6 Daniel Holbert [:dholbert] 2012-01-10 15:00:36 PST
*** Bug 717015 has been marked as a duplicate of this bug. ***
Comment 7 Mounir Lamouri (:mounir) 2012-01-10 15:06:21 PST
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] 2012-01-10 15:11:05 PST
(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] 2012-01-10 15:12:59 PST
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) 2012-01-10 15:16:22 PST
(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] 2012-01-10 15:19:08 PST
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] 2012-01-10 15:34:35 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b40b3847195
Comment 13 Ed Morley [:emorley] 2012-01-11 18:20:16 PST
https://hg.mozilla.org/mozilla-central/rev/5b40b3847195

Note You need to log in before you can comment on or make changes to this bug.