Last Comment Bug 821901 - Fix build warnings in toolkit/components/places
: Fix build warnings in toolkit/components/places
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla20
Assigned To: Catalin Iordache
:
Mentors:
Depends on:
Blocks: buildwarning 819664 823356
  Show dependency treegraph
 
Reported: 2012-12-14 14:57 PST by Catalin Iordache
Modified: 2013-01-22 10:43 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
p4v1.patch (2.31 KB, patch)
2012-12-14 14:57 PST, Catalin Iordache
mak77: review-
Details | Diff | Splinter Review
errors resolved (2.42 KB, patch)
2012-12-17 02:38 PST, Catalin Iordache
mak77: review-
Details | Diff | Splinter Review
final touches (2.41 KB, patch)
2012-12-17 03:14 PST, Catalin Iordache
mak77: review+
Details | Diff | Splinter Review
attachment for checkin (2.51 KB, patch)
2012-12-17 07:38 PST, Catalin Iordache
no flags Details | Diff | Splinter Review
attachment for checkin (2.59 KB, patch)
2012-12-17 07:43 PST, Catalin Iordache
no flags Details | Diff | Splinter Review
bug821901-fix (1.76 KB, patch)
2012-12-18 17:38 PST, Catalin Iordache
mak77: review+
Details | Diff | Splinter Review

Description Catalin Iordache 2012-12-14 14:57:46 PST
Created attachment 692470 [details] [diff] [review]
p4v1.patch

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121129151900

Steps to reproduce:

Fixed build warnings in nsNavHistoryResult.cpp
Comment 1 Matthias Versen [:Matti] 2012-12-15 02:20:46 PST
Comment on attachment 692470 [details] [diff] [review]
p4v1.patch

You have to request review or patches can be missed
Comment 2 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-17 02:08:28 PST
Comment on attachment 692470 [details] [diff] [review]
p4v1.patch

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

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +1626,5 @@
>    uint32_t oldAccessCount = 0;
>    if (!aIsTemporary) {
>      oldAccessCount = mAccessCount;
>      mAccessCount -= mChildren[aIndex]->mAccessCount;
> +    NS_ASSERTION(mAccessCount, "Invalid access count while updating!");

This is wrong, since mAccessCount may be 0.

I think the original purpose of this assertion was to check that mChildren[aIndex]->mAccessCount was not bigger than mAccessCount.

So the assertion should be moved up one line and changed to
NS_ASSERTION(mAccessCount >= mChildren[aIndex]->mAccessCount, "Invalid access count while updating!");

@@ +4028,1 @@
>                   "Invalid index!");

please oneline this if it's below 80 chars
Comment 3 Catalin Iordache 2012-12-17 02:38:20 PST
Created attachment 692891 [details] [diff] [review]
errors resolved

I reviewed my mistakes. Hope this time is better
Comment 4 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-17 02:56:10 PST
Comment on attachment 692891 [details] [diff] [review]
errors resolved

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

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +1626,5 @@
>    uint32_t oldAccessCount = 0;
>    if (!aIsTemporary) {
>      oldAccessCount = mAccessCount;
> +    NS_ASSERTION(mAccessCount >= mChildren[aIndex]->mAccessCount, 
> +                                "Invalid access count while updating!");

Ehr sorry, you should not remove the "mAccessCount -= mChildren[aIndex]->mAccessCount;" line, since that is part of the code needed for this method to do its job, the assertion should just happen before it.

there is a traling space on the first line of the assertion, and the second line should be indented with the ( like

NS_ASSERTION(mAccessCount >= mChildren[aIndex]->mAccessCount,
             "Invalid access count while updating!");
Comment 5 Catalin Iordache 2012-12-17 03:14:45 PST
Created attachment 692899 [details] [diff] [review]
final touches
Comment 6 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-17 03:55:16 PST
Comment on attachment 692899 [details] [diff] [review]
final touches

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

Thank you, this looks good.

You can add the checkin-needed keyword to ask for someone to checkin the patch to the tree for you, it's suggested you add author and checkin comment to it, according to these instructions:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 7 Catalin Iordache 2012-12-17 07:38:53 PST
Created attachment 692946 [details] [diff] [review]
attachment for checkin
Comment 8 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-17 07:41:18 PST
the attachment has author but is missing the checkin comment. who is pushing can add that for now, just recall to add it next time.
Comment 9 Catalin Iordache 2012-12-17 07:43:19 PST
Created attachment 692949 [details] [diff] [review]
attachment for checkin
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-12-17 14:41:08 PST
Comment on attachment 692949 [details] [diff] [review]
attachment for checkin

You can just stick to setting checkin-needed at the top.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-12-17 14:44:57 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd15c87f867

Thanks for the patch!
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-12-17 17:12:21 PST
Sorry, this had to be backed out for causing xpcshell crashes. Please make sure this is green on Try before requesting checkin again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5c8262d399

https://tbpl.mozilla.org/php/getParsedLog.php?id=18030593&tree=Mozilla-Inbound

###!!! ASSERTION: Invalid access count while updating!: 'mAccessCount >= mChildren[aIndex]->mAccessCount', file ../../../../toolkit/components/places/nsNavHistoryResult.cpp, line 1630
<<<<<<<
PROCESS-CRASH | /builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/bookmarks/test_savedsearches.js | application crashed [@ mozalloc_abort(char const*)]
Crash dump filename: /builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/bookmarks/A2FFF43C-BC7C-4B8F-9E59-A70D9756B214.dmp
Operating system: Mac OS X
                  10.8.0 12A269
CPU: amd64
     family 6 model 42 stepping 7
     8 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0

Thread 0 (crashed)
 0  libmozalloc.dylib!mozalloc_abort(char const*) [mozalloc_abort.cpp : 23 + 0x0]
    rbx = 0x00007fff76482c68   r12 = 0x000000010267acca
    r13 = 0x000000010267aca5   r14 = 0x00000001026c84f3
    r15 = 0x00007fff76482c68   rip = 0x0000000100023b64
    rsp = 0x00007fff5fbf98c0   rbp = 0x00007fff5fbf98d0
    Found by: given as instruction pointer in context
 1  XUL!Abort [nsDebugImpl.cpp : 423 + 0x4]
    rbx = 0x00007fff5fbffc65   r12 = 0x000000010267acca
    r13 = 0x000000010267aca5   r14 = 0x00000001026c84f3
    r15 = 0x00007fff76482c68   rip = 0x00000001019cf069
    rsp = 0x00007fff5fbf98e0   rbp = 0x00007fff5fbf98e0
    Found by: call frame info
 2  XUL!NS_DebugBreak_P [nsDebugImpl.cpp : 410 + 0x4]
    rbx = 0x00007fff5fbffc65   r12 = 0x000000010267acca
    r13 = 0x000000010267aca5   r14 = 0x00000001026c84f3
    r15 = 0x00007fff76482c68   rip = 0x00000001019cee3d
    rsp = 0x00007fff5fbf98f0   rbp = 0x00007fff5fbf9d30
    Found by: call frame info
 3  XUL!nsNavHistoryContainerResultNode::RemoveChildAt(int, bool) [nsNavHistoryResult.cpp : 1629 + 0x24]
    rbx = 0x0000000000000000   r12 = 0x000000010453b870
    r13 = 0x000000010453b168   r14 = 0x0000000000000000
    r15 = 0x000000010453b0b0   rip = 0x00000001014ed684
    rsp = 0x00007fff5fbf9d40   rbp = 0x00007fff5fbf9d90
    Found by: call frame info
 4  XUL!nsNavHistoryFolderResultNode::OnItemRemoved(long long, long long, int, unsigned short, nsIURI*, nsACString_internal const&, nsACString_internal const&) [nsNavHistoryResult.cpp : 3807 + 0xc]
    rbx = 0x000000010453b0b0   r12 = 0x0000000102553eb6
    r13 = 0x000000010260bb4e   r14 = 0x0000000000000001
    r15 = 0x0000000000000002   rip = 0x00000001014f6d86
    rsp = 0x00007fff5fbf9da0   rbp = 0x00007fff5fbf9de0
    Found by: call frame info
 5  XUL!nsNavHistoryFolderResultNode::OnItemMoved(long long, long long, int, long long, int, unsigned short, nsACString_internal const&, nsACString_internal const&, nsACString_internal const&) [nsNavHistoryResult.cpp : 4047 + 0x24]
    rbx = 0x000000000000000c   r12 = 0x000000010453b0b0
    r13 = 0x000000000000000b   r14 = 0x0000000105b3dbc0
    r15 = 0x0000000000000000   rip = 0x00000001014f74eb
    rsp = 0x00007fff5fbf9df0   rbp = 0x00007fff5fbf9ee0
    Found by: call frame info
 6  XUL!nsNavHistoryResult::OnItemMoved(long long, long long, int, long long, int, unsigned short, nsACString_internal const&, nsACString_internal const&, nsACString_internal const&) [nsNavHistoryResult.cpp : 4698 + 0x3d]
    rbx = 0x0000000000000000   r12 = 0x000000000000000c
    r13 = 0x0000000000000006   r14 = 0x00007fff5fbf9f48
Comment 13 Catalin Iordache 2012-12-17 18:21:36 PST
is there a way to run this test by myself.. or how could i verify if it's still crashes ?
Comment 14 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-18 01:07:51 PST
interesting, this may be an actual bug, should be verified.

You can run the test using

./mach xpcshell-test toolkit/components/places/tests/bookmarks/test_savedsearches.js

and can debug adding the options --interactive --debug
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-18 01:11:01 PST
You may remove the assertion that is causing the abort (mAccessCount >= mChildren[aIndex]->mAccessCount) and file a separate bug to investigate why it happens. This is not a regression, it's just that the previous version of the assertion was not checkign anything, the new version does its job, and aborts since it fails.
Comment 16 Catalin Iordache 2012-12-18 17:38:04 PST
Created attachment 693703 [details] [diff] [review]
bug821901-fix

Removing the assertion that causes the abort. At that line it's still an warning
Comment 17 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-19 00:58:50 PST
Comment on attachment 693703 [details] [diff] [review]
bug821901-fix

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

Thanks.

Please file a follow-up bug in Toolkit/Places to investigate why mAccessCount may go out of sync and assert when we properly check it.
Comment 18 Catalin Iordache 2012-12-19 18:52:46 PST
I created the bug 823356. A follow-up to this bug
Comment 19 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-20 14:48:10 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9901508358d3

Thanks for your contribution, much appreciated.

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