821901 - Fix build warnings in toolkit/components/places

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Catalin Iordache]

Attachment: 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]]

Comment on attachment 692470 [details] [diff] [review]
p4v1.patch

You have to request review or patches can be missed

Comment 2

[Marco Bonardo [:mak]]

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]

Attachment: errors resolved

I reviewed my mistakes. Hope this time is better

Comment 4

[Marco Bonardo [:mak]]

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]

Attachment: final touches

Comment 6

[Marco Bonardo [:mak]]

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]

Attachment: attachment for checkin

Comment 8

[Marco Bonardo [:mak]]

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]

Attachment: attachment for checkin

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd15c87f867

Thanks for the patch!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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]

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]]

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]]

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]

Attachment: bug821901-fix

Removing the assertion that causes the abort. At that line it's still an warning

Comment 17

[Marco Bonardo [:mak]]

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]

I created the bug 823356. A follow-up to this bug

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9901508358d3

Thanks for your contribution, much appreciated.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9901508358d3