419749 - Add/Update/Delete for items in query set do not seem to close transaction

Status: VERIFIED FIXED

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

Description

[cmtalbert]

Attachment: The "doesn't work" test

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.

Comment 1

[cmtalbert]

Attachment: The "works" test

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

Comment 2

[cmtalbert]

Attachment: A more interesting testcase demonstrating the problem

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.

Comment 3

[cmtalbert]

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.

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[Marco Bonardo [:mak]]

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)

Comment 7

[cmtalbert]

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

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[cmtalbert]

Attachment: A new test that no longer uses setGroupMethod

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.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch

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

[Marco Bonardo [:mak]]

Attachment: patch (fix trailing space)

Comment 12

[Shawn Wilsher :sdwilsh]

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.

Comment 13

[Marco Bonardo [:mak]]

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

Comment 14

[cmtalbert]

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

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch

thank you Clint

Comment 16

[Dietrich Ayala (:dietrich)]

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.

Comment 17

[Marco Bonardo [:mak]]

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

Comment 18

[Marco Bonardo [:mak]]

Attachment: patch

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.

Comment 19

[Marco Bonardo [:mak]]

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)

Comment 20

[Dietrich Ayala (:dietrich)]

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

[Dietrich Ayala (:dietrich)]

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

Comment 22

[Marco Bonardo [:mak]]

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

filled Bug 421897

Comment 23

[Marco Bonardo [:mak]]

(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

Comment 24

[Marco Bonardo [:mak]]

Attachment: patch

removes useless SetPageTitle, all tests PASS

Comment 25

[Dietrich Ayala (:dietrich)]

Comment on attachment 308414 [details] [diff] [review]
patch

r=me

Comment 26

[Dietrich Ayala (:dietrich)]

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

Comment 27

[cmtalbert]

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

--> VERIFIED

Comment 28

[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