452777 - regression from lastInsertRowID: Undo folder deletion mix up views

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

We have a regression with simple str to reproduce:

- create a new profile
- open bookmarks organizer
- go to bookmarks menu folder
- delete mozilla folder
- undo with CTRL-Z

expected result:
- folder is restored

actual result
- folder is restored but the view shows a random element (!?), changing folder and going back shows the correct one

regression-range between 16 Aug and 17Aug builds:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-08-16+00%3A00%3A00&enddate=2008-08-17+03%3A00%3A00

I'm still tracking the bug who caused this, but MAYBE it could also have caused random favicon updates in bug 451499

Comment 1

[Marco Bonardo [:mak]]

regression from bug 449884

Comment 2

[Shawn Wilsher :sdwilsh]

Wow, really?  I don't quite see how on Earth there was any logic change there :/

Comment 3

[Marco Bonardo [:mak]]

probably caused by the fact we are restoring folder maintaing the old Id with CreateContainerWithID, so lastInsertRowId was returning correct result, while now we get the lat id that is wrong

Comment 4

[Shawn Wilsher :sdwilsh]

Oh, snap.  Yeah, that makes sense.  Apparently there was no test coverage on that :/

Comment 5

[Marco Bonardo [:mak]]

i can take this if you want to go on with your work

Comment 6

[Marco Bonardo [:mak]]

i think we shoudl avoid doing this use of rowid since it would be wrong in cases where we insert a row with a fixed id or when you use queries with INSERT OR UPDATE... so imho we should replace queries with direct requests using real data...

Comment 7

[Shawn Wilsher :sdwilsh]

My thoughts as well.  I'm pretty much done with the temp table stuff - need to do some perf testing, but I can take this.

Comment 8

[Marco Bonardo [:mak]]

fine. take a look if somehow (i don't see how) due to this we could end up sending a wrong onItemChanged after we add a new visit. since in bug 451499 we get a itemChanged ATTRIBUTE_FAVICON notification on the view (on a random bookmark) after visiting a site, and is somehow similar to what i'm seeing with undo

Comment 9

[Shawn Wilsher :sdwilsh]

Do you by chance know where the code lives for undo?

Comment 10

[Marco Bonardo [:mak]]

general code is in transaction manager, per folders is in nsnavbookmarks.h

Comment 11

[Shawn Wilsher :sdwilsh]

This is absolutely us returning the wrong id.

Comment 12

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Stupid mistake on my part from the last patch.

There are two different code paths that that method takes, and the original patch only covered one of them.  It's safe to trust the id we are given because the insertion would fail if it wasn't right.

Patch includes a test so this doesn't happen again.

Comment 13

[Shawn Wilsher :sdwilsh]

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

gavin is deferring this review

Comment 14

[Marco Bonardo [:mak]]

why not removing completely the use of MAX(rowid) queries? I know this patch fixes this specific case and is tiny (and for sure r+), but having that queries around could bring to similar problems in future. So i still think we should take the longer way using specific queries, we have good indexes to do that and the cost would not be so bad.

Comment 15

[Shawn Wilsher :sdwilsh]

While I understand your concern, I disagree for a few reasons:
1) The query names don't imply the last insert id, it implies the last id.
2) In order to ensure uniqueness, we'd have to specify several values to ensure we got the right one, which 99% of the time is just going to be the max ROWID anyway.  Since sqlite queries can only use one index per table, this won't be a fast query like it is now.
3) With proper unit testing (which we've been pretty good about enforcing for new code), this issue would get flushed out extremely quickly.  Looking at all the code paths for the previous bug and then looking for tests cases indicates that everything but this path was tested (d'oh!).

Regardless, I think it might be best to open up a new bug to decide that, but get this regression fixed quickly with the patch in this bug.

Comment 16

[Marco Bonardo [:mak]]

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

+/**
+ * This test ensures that when using a folder within a transaction, that undoing
+ * it restores it with the same id.
+ */
+

undoing "using a folder" is not clear, i would revise the comment.
The test also checks that we are sending the right itemId to observers since the problem was that the folder was restored correctly but then we were sending the wrong notification to observers

+  // Create two test folders; remove the first one.  This ensures that undoing
+  // the removal will not get the right id by chance.

not get the right id? clarify the comment please

r=me with more clear comments (you still need a  review from mano or dietrich if you're not a toolkit peer)

i would also add a test that on the restored itemId the title is correct, so it's really our folder.

since the code where we are still using order by ROWID are like "INSERT and immediately get last id" we can leave queries like they are for really minor perf gain, even if i don't like that :)

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> r=me with more clear comments (you still need a  review from mano or dietrich
> if you're not a toolkit peer)
Actually, gavin punted to you and said that you'd be sufficient as a reviewer (since he delegated and is a toolkit peer).  I'll update the patch today and push it.

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses review comments

Comment 19

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b3201f4a8641

Comment 20

[Tracy Walker [:tracy]]

verified: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081112 Minefield/3.1b2pre

Comment 21

[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