448018 - 6 tests report "WARNING: EndUpdateBatch without a begin: file .../nsNavHistoryResult.cpp"

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect, P3)

Description

[Shawn Wilsher :sdwilsh]

../../../../_tests/xpcshell-simple/test_places/bookmarks/test_417228-other-roots.js.log:
>>>>>>>
*** test pending
*** PLACES TESTS: validating Bookmarks Menu
*** PLACES TESTS: validating Bookmarks Toolbar
*** PLACES TESTS: validating Tags
*** PLACES TESTS: validating Unsorted Bookmarks
*** PLACES TESTS: validating test folder
*** PLACES TESTS: validating excluded
WARNING: EndUpdateBatch without a begin: file /Code/moz/central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp, line 4160

Note: the warning should probably be an assertion.

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080817081703 Minefield/3.1a2pre] (home, debug default) (W2Ksp4)

1. |make check|

{{
WARNING: EndUpdateBatch without a begin: file .../toolkit/components/places/src/nsNavHistoryResult.cpp, line 4113
}}

1 warning:
test_384370.js.log
test_417228-other-roots.js.log

2 warnings:
test_405938_restore_queries.js.log
test_417228-exclude-from-backup.js.log
test_424958-json-quoted-folders.js.log
test_restore_guids.js.log

The tests pass, but it would be good to fix nonetheless.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

There is also assertion while running browser-chrome tests:
###!!! ASSERTION: EndUpdateBatch without a begin: 'mBatchInProgress', file /mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp, line 2574

Comment 3

[Dietrich Ayala (:dietrich)]

This should probably not even be a warning. It can happen in perfectly valid circumstances.

Comment 4

[Dietrich Ayala (:dietrich)]

better would be to set mBatchInProgress to true for nodes that are created inside of a batch.

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: v1.0

query doesn't need to be an assertion, and result doesn't need to warn at all (could be created anytime during a batch). this fixes folder nodes to have the proper batch status.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 357044 [details] [diff] [review]
v1.0

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp
>--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
>@@ -3372,8 +3372,6 @@
>     nsresult rv = Refresh();
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
>-  else
>-    NS_WARNING("EndUpdateBatch without a begin");
>   return NS_OK;
> }

if setting mBatchInProgress in nodes generated during a batch is enough to remove the valid circumstances in which it could happen, since we are still warning for queries, we could still leave this as a level of protection from future changes... Does it still warn often?

>@@ -3445,6 +3443,8 @@
>            itemType == nsINavBookmarksService::TYPE_DYNAMIC_CONTAINER) {
>     rv = bookmarks->ResultNodeForContainer(aItemId, mOptions, getter_AddRefs(node));
>     NS_ENSURE_SUCCESS(rv, rv);
>+    if (node->IsFolder())
>+      node->GetAsFolder()->mBatchInProgress = mBatchInProgress;

is there a reason to skip dynamic containers with that isFolder check?

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 357044 [details] [diff] [review]
v1.0

revising this, patch is ok as it is, i'll look into dynamic containers directly while working on them.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 357044 [details] [diff] [review]
v1.0

btw, mBatchInProgress is only defined for nsNavHistoryResult and nsNavHistoryQueryResultNode, so should probably be assigned to the node asQuery and to its result, since this won't compile :\

Comment 9

[Marco Bonardo [:mak]]

i fixed a couple of this in a previous patch, but iirc i still saw one of this warnings, so probably we need to re-triage this, checking which tests still activate the warning, and making sure mBatchInProgress is correctly setup on nodes creation

Comment 10

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a2pre) Gecko/20090812 Minefield/3.6a2pre] (home, debug default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/6470caf066b2)

'check' and 'xpcshell-tests' don't report this warning anymore.
Do "reftests" and "mochitests" need to be checked too?

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> There is also assertion while running browser-chrome tests:
> ###!!! ASSERTION: EndUpdateBatch without a begin: 'mBatchInProgress', file
> /mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp, line 2574

Oh, I didn't remembered that one.
Do you still see it?


(In reply to comment #10)
> Do "reftests" and "mochitests" need to be checked too?

(Anyway, I get a crash on startup so I can't run them... :-/)

Comment 12

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 13

[Marco Bonardo [:mak]]

most of these should have been fixed, we should file single bugs on the remaining ones (if there is any)