Closed Bug 452777 Opened 11 years ago Closed 11 years ago

regression from lastInsertRowID: Undo folder deletion mix up views

Categories

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

defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 3.1b1

People

(Reporter: mak, Assigned: sdwilsh)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(1 file, 1 obsolete file)

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
Flags: blocking-firefox3.1?
regression from bug 449884
Blocks: 449884
Keywords: regression
Wow, really?  I don't quite see how on Earth there was any logic change there :/
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
Oh, snap.  Yeah, that makes sense.  Apparently there was no test coverage on that :/
i can take this if you want to go on with your work
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...
Summary: regression: Undo folder deletion mix up views → regression from lastInsertRowID: Undo folder deletion mix up views
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.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Whiteboard: [needs patch]
Blocks: 430442
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
Do you by chance know where the code lives for undo?
general code is in transaction manager, per folders is in nsnavbookmarks.h
This is absolutely us returning the wrong id.
Attached patch v1.0 (obsolete) — Splinter Review
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.
Attachment #336122 - Flags: review?(gavin.sharp)
Flags: in-testsuite?
Flags: in-litmus-
Hardware: PC → All
Whiteboard: [needs patch] → [has patch][needs review gavin]
Attachment #336122 - Flags: review?(gavin.sharp) → review?(mak77)
Comment on attachment 336122 [details] [diff] [review]
v1.0

gavin is deferring this review
Whiteboard: [has patch][needs review gavin] → [has patch][needs review Marco]
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.
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.
Attachment #336122 - Flags: review?(mak77) → review+
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 :)
(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.
Whiteboard: [has patch][needs review Marco] → [needs new patch]
Attached patch v1.1Splinter Review
Addresses review comments
Attachment #336122 - Attachment is obsolete: true
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b3201f4a8641
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs new patch]
Target Milestone: --- → Firefox 3.1b1
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Target Milestone: Firefox 3.1b1 → Firefox 3.1
verified: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081112 Minefield/3.1b2pre
Status: RESOLVED → VERIFIED
Target Milestone: Firefox 3.1 → Firefox 3.1b1
Keywords: verified1.9.1
Keywords: fixed1.9.1
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.