Closed Bug 462379 Opened 11 years ago Closed 11 years ago

(NS_ERROR_FAILURE) [nsINavBookmarksService.insertBookmark] when trying to star a bookmark

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: marcia, Assigned: sdwilsh)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(1 file, 4 obsolete files)

Seen while running  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081030 Minefield/3.1b2pre. Does not seem to happen using Win XP equivalent build. I am using a new profile.

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsINavBookmarksService.insertBookmark]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///Users/marcia/Desktop/Latest%20Trunk/Minefield.app/Contents/MacOS/components/nsPlacesTransactionsService.js :: PCIT_doTransaction :: line 377"  data: no]
Flags: blocking-firefox3.1?
hrm, after I tried to go back and reproduce this I have not yet been able to. Will keep trying.
I don't expect you to be able to reproduce this consistently.  After talking with asuth for a few hours this morning, we think that this is the problem:

When we run our sync of both historyvistits and places on the background, we use the multiple statement API.  However, due to bugs like bug 462173, we can end up starting a transaction but never finishing it.  We end up not finishing it if we have a select statement that has not yet been reset on the main thread.

Additionally, there is a bug with the transaction code in mozStorageConnection that sets mTransactionInProgress to false even if an error occurred, resulting in the next use of mozStorageTransaction to succeed in obtaining a transaction, when it doesn't actually have one.

When we go ahead and try to commit our transaction on the main thread, it ends up failing, making this operation fail as well.

The simple fix here is to not use the multiple statement api when we sync both tables, and to dispatch them both as one-offs.
Attached patch v1.0 (obsolete) — Splinter Review
Here is that quick fix.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #345553 - Flags: review?(dietrich)
Attached patch v1.0 (obsolete) — Splinter Review
silly shawn - tricks are for kids
Attachment #345553 - Attachment is obsolete: true
Attachment #345557 - Flags: review?(dietrich)
Attachment #345553 - Flags: review?(dietrich)
I'm getting these errors:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsINavBookmarksService.insertBookmark]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Firefox%20-%20Trunk/components/nsPlacesTransactionsService.js :: PCIT_doTransaction :: line 377"  data: no]

and

Error: Async statement execution returned with '1', 'Transaction failed to commit'
Source File: file:///C:/Program%20Files/Mozilla%20Firefox%20-%20Trunk/components/nsPlacesDBFlush.js
Line: 172

I'm using:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081030 Minefield/3.1b2pre
Attached patch v1.2 (obsolete) — Splinter Review
I am full of fail today
Attachment #345557 - Attachment is obsolete: true
Attachment #345564 - Flags: review?(dietrich)
Attachment #345557 - Flags: review?(dietrich)
Attachment #345564 - Flags: review?(dietrich) → review+
Comment on attachment 345564 [details] [diff] [review]
v1.2

r=me, thanks
(In reply to comment #5)
> I'm getting these errors:
What you are seeing is exactly what I described in comment 2
http://hg.mozilla.org/mozilla-central/rev/812455b0e56a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
OS: Mac OS X → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
Duplicate of this bug: 462164
tests are failing - I know why.  It's actually correct, but we should fix things slightly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v2.0 (obsolete) — Splinter Review
This will batch the two completions we receive into one, as the code used to work.
Attachment #345606 - Flags: review?(dietrich)
Attachment #345606 - Flags: review?(dietrich) → review+
Pushed extra followup:
http://hg.mozilla.org/mozilla-central/rev/74750cf8a8a7

       6          this._count++;
       7 -        if (this._count == 1) {
       8 +        if (this._count == 2) {
       9            // we have gotten both notifications
(In reply to comment #14)
Quoted for emphasis:
> I am full of fail today
Tests still failing on the tinderbox. I backed everything out.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Priority: -- → P1
mh, why are you checking on a fixed number of statements, the function actually could have one or two statements based on the fact we are syncing only moz_places or both tables, and the check for a fixed count does not make sense, you must use statements.length.
i would tell initialize _count to statements length and decrease till 0, should be less error prone.
yeah, the solution in comment 18 is the correct one.  I clearly was not thinking correctly yesterday.
Attached patch v2.1Splinter Review
Attachment #345564 - Attachment is obsolete: true
Attachment #345606 - Attachment is obsolete: true
Attachment #345747 - Flags: review?(sdwilsh)
Comment on attachment 345747 [details] [diff] [review]
v2.1

r=sdwilsh
Attachment #345747 - Flags: review?(sdwilsh) → review+
checked in: http://hg.mozilla.org/mozilla-central/rev/944d15e482ba

will mark fixed after confirming the tests still pass.
Tests have been passing, so I'm going to resolve this.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
is this error related? i'm still seeing this when i add a bookmark: 

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]
It shouldn't be - Marco added code to make sure our QI is correct now...
verified: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081112 Minefield/3.1b2pre

was there a new bug filed on comment #24 error.  I see it in the above build.
Status: RESOLVED → VERIFIED
(In reply to comment #26)
> verified: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre)
> Gecko/20081112 Minefield/3.1b2pre
> 
> was there a new bug filed on comment #24 error.  I see it in the above build.

filed bug 464471
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.