1049747 - History.cpp:931:19: warning: unused variable 'navHistory' [-Wunused-variable] (and another similar unused variable further down)

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Daniel Holbert [:dholbert]]

Linux opt build warning:
{
/toolkit/components/places/History.cpp: In constructor 'mozilla::places::{anonymous}::InsertVisitedURIs::InsertVisitedURIs(mozIStorageConnection*, nsTArray<mozilla::places::VisitData>&, mozIVisitInfoCallback*)':
/toolkit/components/places/History.cpp:931:19: warning: unused variable 'navHistory' [-Wunused-variable]

/toolkit/components/places/History.cpp: In member function 'virtual nsresult mozilla::places::History::GetPlacesInfo(JS::Handle<JS::Value>, mozIVisitInfoCallback*, JSContext*)':
/toolkit/components/places/History.cpp:2695:17: warning: unused variable 'navHistory' [-Wunused-variable]
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=45321945&tree=Try

These are both just unused variables that should be dropped. (They're only "used" in debug builds only because we have a null-check assertion.)

For the record, the first variable's only usage was removed here (for bug 561450), leaving it unused:
https://hg.mozilla.org/mozilla-central/diff/69f4d3ba11a5/toolkit/components/places/History.cpp#l1.96

...and the second variable was never used in the first place. It was added with no usages here (for bug 834539):
http://hg.mozilla.org/mozilla-central/diff/734147446def/toolkit/components/places/History.cpp#l1.910

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> https://tbpl.mozilla.org/php/getParsedLog.php?id=45321945&tree=Try

(That Try log shows these build warnings treated as errors, since it had a patch in the queue to add FAIL_ON_WARNINGS to this directory.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix: drop the unused variables

Comment 3

[Daniel Holbert [:dholbert]]

(mak: if you happen to notice this before mano, feel free to steal the review. Trivial patch; just removing two unused variables.)

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8468613 [details] [diff] [review]
fix: drop the unused variables

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

I'm pretty sure those exist because we need to be sure history service is up and running before proceeding.
I think you rather want DebugOnly here, MOZ_ASSERT, and add a comment.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #4)
> I'm pretty sure those exist because we need to be sure history service is up
> and running before proceeding.

I don't think I agree.

I can interpret your comment in two ways:
 (1) We need to check if GetHistoryService() fails, and bail out if it does because we can't usefully proceed.

If this is what you mean, I disagree that this is useful, because we don't actually bail out on failure right now (we just assert, in debug builds).  So we're not usefully reacting to  GetHistoryService() failures here.  So I don't see any reason to call it in opt builds; it's just useless overhead.

OR, if your meaning was:
 (2) We need to call GetHistoryService() to make sure some state is set up for something to use later.

If this is what you mean, I think I disagree with this, too. GetHistoryService() seems to return a lazy-initialized variable. So if something is going to use it later, then we can do the lazy-initialization when that later thing tries to get it.  No need to prematurely force the lazy-initialization.

Am I missing something?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> I can interpret your comment in two ways:
>  (1) We need to check if GetHistoryService() fails, and bail out if it does
> because we can't usefully proceed.
> 
> If this is what you mean, I disagree that this is useful, because we don't
> actually bail out on failure right now (we just assert, in debug builds). 
> So we're not usefully reacting to  GetHistoryService() failures here.  So I
> don't see any reason to call it in opt builds; it's just useless overhead.

OK, so the fact is we will need history service later, but at that time we are on another thread so we cannot initialize it.
I agree we might want to handle errors better, even if this is very unlikely to be hit.

In GetPlacesInfo let's null check history and return error.
In InsertVisitedURIs we are in a constructor so we can't return an error. Let's move the check into InsertVisitedURIs::Start and return an error from there.

Does this work better for you?

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #6)
> OK, so the fact is we will need history service later, but at that time we
> are on another thread so we cannot initialize it.

Ah, this makes more sense. Thanks for clarifying.

> In GetPlacesInfo let's null check history and return error.
> In InsertVisitedURIs we are in a constructor so we can't return an error.
> Let's move the check into InsertVisitedURIs::Start and return an error from
> there.
> 
> Does this work better for you?

I think so -- I'll give that a shot. Thanks!

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2: bail if navHistory is null

Patch updated per comment 6.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8470102 [details] [diff] [review]
fix v2: bail if navHistory is null

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

Thank you

Comment 10

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5709d54523ef

Comment 11

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/5709d54523ef