Closed Bug 992670 Opened 6 years ago Closed 6 years ago

Fix always true assertion condition in nsNavHistoryResult.cpp

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: poiru, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 2 obsolete files)

GCC warning:
toolkit/components/places/nsNavHistoryResult.cpp:1460:32 [-Wtype-limits] comparison of unsigned expression >= 0 is always true
Comment on attachment 8402374 [details] [diff] [review]
Fix always true assertion condition in nsNavHistoryResult.cpp

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

Unfortunately, the right assertion is being hit, so we can't fix it until the code causing it is fixed.

This seems to be due to the aIsTemporary argument of some methods, that skips accessCount recalculations...
InsertSortedChild passes true for aIsTemporary in 2 cases:
1. inside onTitleChanged. In this case we are not just moving a node, we are adding a new node, so it should not pass true
2. inside notifyIfTagsChanged. again, we are adding a new node, so it should not pass true

that means basically this argument is completely misused, I'm not surprised, since it looks like a foot gun.

So, I'm going to ask, would you like to fix this mess, or do you think it's too much scope-creep for you?
What should be done:
1. fix the assertion, as you did, but use MOZ_ASSERT for it
2. Remove aIsTemporary argument from RemoveChildAt, InsertChildAt, InsertSortedChild (fix callpoints and internal code to assume it's always false)
3. Try server run to check all existing tests are passing
Attachment #8402374 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #2)
> 1. fix the assertion, as you did, but use MOZ_ASSERT for it

Done.
Attachment #8402374 - Attachment is obsolete: true
Attachment #8405738 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #2)
> 2. Remove aIsTemporary argument from RemoveChildAt, InsertChildAt,
> InsertSortedChild (fix callpoints and internal code to assume it's always
> false)
> 3. Try server run to check all existing tests are passing

Done. Try push: https://tbpl.mozilla.org/?tree=Try&rev=ceecd95e6074
Attachment #8405739 - Flags: review?(mak77)
Comment on attachment 8405738 [details] [diff] [review]
Fix always true assertion condition in nsNavHistoryResult.cpp

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

this one looks good.
Attachment #8405738 - Flags: review?(mak77) → review+
Comment on attachment 8405739 [details] [diff] [review]
Make all child insertions/removals non-temporary in nsNavHistoryContainerResultNode

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

and this one looks good as well.

Thank you for doing this!

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +1455,3 @@
>      nsresult rv = ReverseUpdateStats(mAccessCount - oldAccessCount);
>      NS_ENSURE_SUCCESS(rv, rv);
>      oldNode->OnRemoving();

this code should be re-indented properly
Attachment #8405739 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] from comment #6)
> this code should be re-indented properly

Fixed.
Attachment #8405739 - Attachment is obsolete: true
Attachment #8407593 - Flags: review+
Keywords: checkin-needed
not a crash, we are still hitting the assertion...
the failing test is browser_410196_paste_into_tags.js that should help figuring where we are miscalculating stats, I suspect some of the "bypass" we do for tag containers.
Flags: needinfo?(birunthan)
(Just a heads-up: I believe Ms2ger is planning on commenting out the existing (trivially satisfied) assertion, so that we can make "-Wtype-limits" globally treated as an error. That will prevent us from introducing more bugs like this one & bug 1012814.

There may be a more useful thing we should be asserting in this code (and it sounds like we may not yet be able to safely assert that more-useful thing, per comment 9).  But in the meantime, there's no value in having the current assertion (and to the extent that it blocks us from catching other Wtype-limits bugs, its existence imposes a nonzero cost).)
Version: unspecified → Trunk
If I recall correctly, something in MoveItem was causing at least some of the assertions. I don't have time to dig deeper into this at the moment, so unassigning for now. I'll take another look when I can.

(In reply to Daniel Holbert [:dholbert] from comment #11)
> (Just a heads-up: I believe Ms2ger is planning on commenting out the
> existing (trivially satisfied) assertion, so that we can make
> "-Wtype-limits" globally treated as an error. That will prevent us from
> introducing more bugs like this one & bug 1012814.

Thanks for the info!
Assignee: birunthan → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(birunthan)
it's fine to disable the assertion for now, we should investigate what causes the "real" assertion to be hit yet.
I will tryrun the second patch here and if it still applies I will land it, cause that part is a nice cleanup to have and solves part of the issues.
Flags: needinfo?(mak77)
Duplicate of this bug: 1022699
(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #14)
> I will tryrun the second patch here and if it still applies I will land it,
> cause that part is a nice cleanup to have and solves part of the issues.

FWIW, the second patch does still apply, though it layers on top of patch 1. (So, you get rejects if you apply it without first applying patch 1.)

Also, it looks like Ms2ger didn't end up making -Wtype-limits fatal (yet) or disabling this assertion (per comment 11/12).  So, we're still failing this build warning.

I think we should:
 (A) File a followup to investigate why the assertion is failing.
 (B) Re-land the patches that landed here, with an additional patch to label the corrected-but-failing assertion (from part 1) commented out, with an XXX comment referencing the followup bug.

This will let us proceed with labeling the directory as FAIL_ON_WARNINGS, to catch future bugs easier.

Mak, does this sound OK to you?

To get the ball rolling, I've pushed a try run with the original patches here (un-modified), to be sure we're still having issues:
 https://tbpl.mozilla.org/?tree=Try&rev=33a80a12115a
(Though Try was closed shortly after I pushed, so that push may not be useful.)
(In reply to Daniel Holbert [:dholbert] from comment #16)
> I think we should:
>  (A) File a followup to investigate why the assertion is failing.

I've just filed bug 1049797 on this, FWIW.
Comment on attachment 8468671 [details] [diff] [review]
part 3: disable assertion, w/ link to bug 1049797

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

yes please, it's a long time I should have done this but I constantly failed at organizing stuff :/
Much appreciated.
Attachment #8468671 - Flags: review?(mak77) → review+
Flags: needinfo?(mak77)
Thanks for the review!

Also, "good news" -- my Try run (with just the original patches, not the 3rd commenting-out patch) did hit the assertion-failure, on debug bc1, in browser_410196_paste_into_tags.js  (the same test noted in comment 10).

Once Try has reopened, I'll do a Try push with the third patch as well, to make sure nothing else is broken once we turn off this assertion.
Duplicate of this bug: 823356
You need to log in before you can comment on or make changes to this bug.