While trying to write tests for bug 384226, I came across this scenario. After much debugging, I can't seem to find anything wrong with my code. It appears that performing a batch update causes some sort of transaction to close, resulting in unbatched insert/update/delete operations to finalize and appear in a live query set. I am attaching two tests to demonstrate the issue. You would expect that when adding/updating/deleting items in the query set, you would see the number of items in the query set change on completion of the operation. However, these numbers (and indeed the operations) do not seem to take effect until the batch update is run. In the -doesntwork test, you can see that from the do_equal_check that the number of items in the query set is not working. In this test, you can also see that the change to the query set fro the add and update are only applied once the update batch is run. The "deletion" from the query set is never taken into account. In the -works test, the delete is above the update batch call and you can see that all operations on the query set have succeeded once the update batch call have succeeded.

Note that these tests are set up to run within the "queries" infrastructure available from bug 384226

This test case is even more interesting and attempts to use several types of changes to get things included in the result set via live update. But you can see from running it (it fails in order to produce debug output) that the result set after the changes completely matches the result set before the changes. If you uncomment the batch update portion of the test, you will see that all the items are then added to the result set properly as you'd expect. If you move the batch update section to occur prior to any of the above changes, you will find that only the changes occurring before the batch update will get reflected into the live update of the query result set. This just smells a lot like a transaction problem.

Spoke with Dietrich about this bug in email. He recommended that I nominate it for blocking. So I: Set priority to: P1 Set Target Milestone to: Firefox 3 And set blocking-Firefox3 to: ? I think that the live update query sets of the places interfaces are a pretty awesome critical feature, and if something as simple as these tests don't work, then I think we'll really run a risk of introducing a core source of fragility in our feature if we don't fix this. Therefore, it should block.

could you try if this solves your problem? for me it is solved, we are checking if domainishost is true, if it's false we don't update and that does not make sense to me

adding a batch update solves the problem because it calls Refresh() when doing onEndUpdateBatch. so that should explain the behaviour.

PS: you should probably change setGroupingMode if i'm not wrong grouping has gone, and please don't rely on SetPageDetails to set the visit_count since that will most likely change (there should not be a way to set the visit_count by choice, it should be maintained correct by the service itself)

(In reply to comment #4) > Created an attachment (id=307435) [details] > test patch > > could you try if this solves your problem? for me it is solved, we are checking > if domainishost is true, if it's false we don't update and that does not make > sense to me > Ok, tried it. With the Domain query option and this patch, I get the following behavior: * Add an item to query set --> works * Update an item to add it to query set --> doesn't work * Delete an item from the query set --> doesn't work Without the Domain option specified on the query at all, I get the following behavior: * Add an item to query set --> doesn't work * Update an item to add it to query set --> doesn't work * Delete an item from the query set --> doesn't work Note that this same problem (unable to reflect changes to the query set) also occurs if you use the "uri" option in place of the domain option, I'm not sure if that's important to note or not. I will definitely look into my complex test and remove the groupingMode setting. Once I have a new test, I'll attach it here. Thanks for the feedback on the test, I appreciate it. Let me know when you have another patch to test, and I'll be happy to try it out.

title updating is failing because we are not calling observers into setPageDetails, fixed locally

Here is a new test for the issue. Summary of changes: * Removed setGroup and the for loops to walk through the resulting Group Tree * Added a test to delete something from the result set by changing the title * Cleaned up the debug output * Made sure to always use a visit count of 1 since we shouldn't depend on setPageDetails to change visit counts (the test already didn't query on visit count so it was silly to vary them in the first place). One thing I noticed while working with this new test and the Test patch (attachment 307435 [details] [diff] [review]): When I performed an update by adding a new visit during the specified time range (i.e. the change done for the http://foo.com/changeme1 URI), then this URI appeared in the result set. Once I backed out the test patch, then this URI stopped appearing in the result set. In the tests above, I was testing update by updating the title of the node, so this might be an important distinction. So, the test patch is an important first step, and perhaps for more than simply adding a new node.

// Let's update something by annotation changedURI = uri("http://foo.com/badannotaion.html"); annosvc.setPageAnnotation(changedURI, "text/mozilla", "test", 0, 0); this doesn't make sense to me, you are setting an anno into an item that is not included into the resultset, you will not see anything... You should instead do annosvc.removePageAnnotation(changedURI, "text/foo"); and see if the result come back Hwv, i can't reproduce your last problems, with your test and this patch (that simply adds support to setPageDetails and fix the previous problem with host) i can see titles updated correctly, add and delete are correct, the exiting resultset is what i expect (you should add titles to dump so you can see the changes)

Comment on attachment 307675 [details] [diff] [review] patch (fix trailing space) >- if (! mExpanded) { >+ if (!mExpanded) { >- for (PRInt32 i = 0; i < matches.Count(); i ++) { >+ for (PRInt32 i = 0; i < matches.Count(); ++i) { Both of these changes seem quite unnecessary and just lose valuable blame information.

yeah, style is mixed up in that file. will revert after knowing if the problem is solved for Clint.

(In reply to comment #13) > yeah, style is mixed up in that file. will revert after knowing if the problem > is solved for Clint. > This patch solves the problems. Nice work!

thank you Clint

http://mxr.mozilla.org/seamonkey/search?string=setpagedetails Note that currently we have no internal consumers of setPageDetails, besides unit tests utilizing it for it's dangerous immediate db update. IIRC, it was originally designed for use during import. We've talked about removing it entirely. Secondly, I think the notification you added is not correct, since you don't know the title changed from what was previously there. I think that the proper fix here might be to remove that API, and instead use the other nsINavHistoryService/nsIBrowserHistory/nsIGlobalHistory APIs to achieve the objective in the tests.

i agree with removing that api, it's quite dangerous, thank you dietrich

this removes setPageDetails, and changes tests accordingly (all tests PASS) Notice however that now we have no way to change a page title immediately, because SetPageTitle uses a LAZY_ADD, so the title could be changed after our check. The only way to set the title immediately is using AddPageWithDetails, but still that requires that we also need to add a visit (transition_link). We could modify AddPageWithDetails so that it adds a visit only if (aLastVisited), and maybe add a 4th param as transition argument since we could want to add a first visit that is not a transition_link.

Also, as a third party user, i could expect that setPageTitle is an immediate change, since in the nsIGlobalHistory2 interface definition there's no comment about lazy mode (http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIGlobalHistory2.idl#91)

(In reply to comment #19) > Also, as a third party user, i could expect that setPageTitle is an immediate > change, since in the nsIGlobalHistory2 interface definition there's no comment > about lazy mode > (http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIGlobalHistory2.idl#91) > please file a followup issue about setting the page title.

Comment on attachment 308391 [details] [diff] [review] patch >Index: toolkit/components/places/tests/unit/test_adaptive.js >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_adaptive.js,v >retrieving revision 1.2 >diff -u -8 -p -r1.2 test_adaptive.js >--- toolkit/components/places/tests/unit/test_adaptive.js 28 Feb 2008 16:04:59 -0000 1.2 >+++ toolkit/components/places/tests/unit/test_adaptive.js 10 Mar 2008 09:55:07 -0000 > > function setCountRank(aURI, aCount, aRank, aSearch) > { >- // Set the visit count and date for a uri >- histsvc.setPageDetails(aURI, aURI, aCount, false, true); >- > // Bump up the visit count for the uri > for (let i = 0; i < aCount; i++) > histsvc.addVisit(aURI, d1, null, histsvc.TRANSITION_TYPED, false, 0); > >+ // Set the page title >+ ghist.setPageTitle(aURI, aURI); >+ should be aURI.spec for title. that's not failing? >Index: toolkit/components/places/tests/unit/test_frecency.js >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_frecency.js,v >retrieving revision 1.3 >diff -u -8 -p -r1.3 test_frecency.js >--- toolkit/components/places/tests/unit/test_frecency.js 26 Feb 2008 00:22:09 -0000 1.3 >+++ toolkit/components/places/tests/unit/test_frecency.js 10 Mar 2008 09:55:07 -0000 > function setCountDate(aURI, aCount, aDate) > { >- // Set the visit count and date for a uri >- histsvc.setPageDetails(aURI, aURI, aCount, false, true); > // We need visits so that frecency can be computed over multiple visits > for (let i = 0; i < aCount; i++) > histsvc.addVisit(aURI, aDate, null, histsvc.TRANSITION_TYPED, false, 0); >+ >+ // Set the page title >+ ghist.setPageTitle(aURI, aURI); > } ditto

(In reply to comment #20) > please file a followup issue about setting the page title. filled Bug 421897

(In reply to comment #21) > should be aURI.spec for title. that's not failing? that's interesting, no that's not failing, plus this error was already there (setPageDetails) so i did not pay attention to that :( Probably i can completely remove the setPageTitle from there, it should not be used in those tests

removes useless SetPageTitle, all tests PASS

Comment on attachment 308414 [details] [diff] [review] patch r=me

Checking in toolkit/components/places/public/nsINavHistoryService.idl; /cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v <-- nsINavHistoryService.idl new revision: 1.80; previous revision: 1.79 done Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.271; previous revision: 1.270 done Checking in toolkit/components/places/src/nsNavHistory.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHistory.h new revision: 1.147; previous revision: 1.146 done Checking in toolkit/components/places/src/nsNavHistoryResult.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v <-- nsNavHistoryResult.cpp new revision: 1.136; previous revision: 1.135 done Checking in toolkit/components/places/tests/unit/test_000_frecency.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_000_frecency.js,v <-- test_000_frecency.js new revision: 1.7; previous revision: 1.6 done Checking in toolkit/components/places/tests/unit/test_416211.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_416211.js,v <-- test_416211.js new revision: 1.2; previous revision: 1.1 done Checking in toolkit/components/places/tests/unit/test_417798.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_417798.js,v <-- test_417798.js new revision: 1.2; previous revision: 1.1 done Checking in toolkit/components/places/tests/unit/test_adaptive.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_adaptive.js,v <-- test_adaptive.js new revision: 1.3; previous revision: 1.2 done Checking in toolkit/components/places/tests/unit/test_frecency.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_frecency.js,v <-- test_frecency.js new revision: 1.4; previous revision: 1.3 done Checking in toolkit/components/places/tests/unit/test_multi_word_search.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_multi_word_search.js,v <-- test_multi_word_search.js new revision: 1.3; previous revision: 1.2 done Checking in toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js,v <-- test_resolveNullBookmarkTitles.js new revision: 1.5; previous revision: 1.4 done

Verified with debug build pulled from 3/18/08. (by running the test above, which was changed to no longer use setPageDetails). --> VERIFIED

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