Fix build warnings in toolkit/components/places

RESOLVED FIXED in mozilla20

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Catalin Iordache, Assigned: Catalin Iordache)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Hardware: x86 → All
Comment on attachment 692470 [details] [diff] [review]
p4v1.patch

You have to request review or patches can be missed
Attachment #692470 - Flags: review?(mak77)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Attachment #692470 - Flags: review?(mak77) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 692891 [details] [diff] [review]
errors resolved

I reviewed my mistakes. Hope this time is better
Attachment #692470 - Attachment is obsolete: true
Attachment #692891 - Flags: review?(mak77)
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!");
Attachment #692891 - Flags: review?(mak77) → review-
(Assignee)

Comment 5

5 years ago
Created attachment 692899 [details] [diff] [review]
final touches
Attachment #692891 - Attachment is obsolete: true
Attachment #692899 - Flags: review?(mak77)
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
Attachment #692899 - Flags: review?(mak77) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 692946 [details] [diff] [review]
attachment for checkin
Attachment #692946 - Flags: checkin?
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.
Keywords: checkin-needed
(Assignee)

Comment 9

5 years ago
Created attachment 692949 [details] [diff] [review]
attachment for checkin
Attachment #692946 - Attachment is obsolete: true
Attachment #692946 - Flags: checkin?
Attachment #692949 - Flags: checkin?
Attachment #692899 - Attachment is obsolete: true
Comment on attachment 692949 [details] [diff] [review]
attachment for checkin

You can just stick to setting checkin-needed at the top.
Attachment #692949 - Flags: checkin?
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd15c87f867

Thanks for the patch!
Assignee: nobody → catalinn.iordache
Keywords: checkin-needed
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
(Assignee)

Comment 13

5 years ago
is there a way to run this test by myself.. or how could i verify if it's still crashes ?
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
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.
Blocks: 819664
(Assignee)

Comment 16

5 years ago
Created attachment 693703 [details] [diff] [review]
bug821901-fix

Removing the assertion that causes the abort. At that line it's still an warning
Attachment #692949 - Attachment is obsolete: true
Attachment #693703 - Flags: review?(mak77)
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.
Attachment #693703 - Flags: review?(mak77) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Blocks: 823356
(Assignee)

Comment 18

5 years ago
I created the bug 823356. A follow-up to this bug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9901508358d3

Thanks for your contribution, much appreciated.
Keywords: checkin-needed
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/9901508358d3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 187528
Component: General → Places
Product: Core → Toolkit
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.