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 on attachment 692470 [details] [diff] [review] p4v1.patch You have to request review or patches can be missed

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

I reviewed my mistakes. Hope this time is better

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

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 on attachment 692949 [details] [diff] [review] attachment for checkin You can just stick to setting checkin-needed at the top.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd15c87f867 Thanks for the patch!

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

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.

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

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.

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.