Closed
Bug 992670
Opened 10 years ago
Closed 10 years ago
Fix always true assertion condition in nsNavHistoryResult.cpp
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: poiru, Unassigned)
References
(Blocks 2 open bugs)
Details
Attachments
(3 files, 2 obsolete files)
1.34 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
11.28 KB,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
1.38 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
GCC warning: toolkit/components/places/nsNavHistoryResult.cpp:1460:32 [-Wtype-limits] comparison of unsigned expression >= 0 is always true
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8402374 -
Flags: review?(mak77)
Comment 2•10 years ago
|
||
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)
Reporter | ||
Comment 3•10 years ago
|
||
(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)
Reporter | ||
Comment 4•10 years ago
|
||
(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 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Reporter | ||
Comment 7•10 years ago
|
||
(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+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 8•10 years ago
|
||
Backed out for mochitest-bc and xpcshell crashes. Please make sure this has a clean Try run before re-requesting checkin. https://hg.mozilla.org/integration/mozilla-inbound/rev/c60b059c47ca https://tbpl.mozilla.org/php/getParsedLog.php?id=37944730&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=37944614&tree=Mozilla-Inbound
Comment 9•10 years ago
|
||
not a crash, we are still hitting the assertion...
Comment 10•10 years ago
|
||
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.
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Flags: needinfo?(birunthan)
Comment 11•10 years ago
|
||
(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).)
Updated•10 years ago
|
Version: unspecified → Trunk
Reporter | ||
Comment 12•10 years ago
|
||
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)
Comment 13•10 years ago
|
||
it's fine to disable the assertion for now, we should investigate what causes the "real" assertion to be hit yet.
Comment 14•10 years ago
|
||
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)
Updated•10 years ago
|
Blocks: buildwarning
Comment 16•10 years ago
|
||
(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.)
Comment 17•10 years ago
|
||
(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 18•10 years ago
|
||
Attachment #8468671 -
Flags: review?(mak77)
Comment 19•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: needinfo?(mak77)
Comment 20•10 years ago
|
||
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.
Comment 21•10 years ago
|
||
Try run with all 3 patches: https://tbpl.mozilla.org/?tree=Try&rev=1490cc1d22b5
Comment 22•10 years ago
|
||
Landed! Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce7bbd7598e Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/773d3d139fb5 Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/e57fb96938ea
Flags: in-testsuite-
Comment 23•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4ce7bbd7598e https://hg.mozilla.org/mozilla-central/rev/773d3d139fb5 https://hg.mozilla.org/mozilla-central/rev/e57fb96938ea
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•