405497 - Calling RunInBatchedMode within a RunBatched implementation hangs

Status: RESOLVED FIXED

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

Description

[Asaf Romano (gone)]

I found this issue while working on bug 401753.

Calling RunInBatchedMode within a RunBatched implementation hangs. This very likely happens due the lock that's used there to block parallel calls :(

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Yay!  Locks are so much fun!

This patch needs an explanation to make reviewing easier.

First, I fixed the XXX in browser/components/nsBrowserGlue.js about this bug.

Now for the good part.  This hold true for both nsNavBookmarks and nsNavHistory.  While it is true that we could have used a PR_Monitor instead of a PR_Lock (it is reentrant), I determined that that still would not be ideal since other code makes calls to [Begin|End]BatchUpdate.  So now we lock when we would try to get a transaction, and unlock when we would complete the transaction.

Comment 2

[Shawn Wilsher :sdwilsh]

<timeless> sdwilsh: err, holding a lock while calling outside your module is illegal

So, my question is, why were we locking in the first place?  It is still not immediately clear to me...

Comment 3

[timeless]

 101 NS_IMPL_ISUPPORTS3(nsNavBookmarks,
 102                    nsINavBookmarksService,
 103                    nsINavHistoryObserver,
 104                    nsIAnnotationObserver)

the class isn't threadsafe. Locks and higher order things should only be used for threadsafe objects to protect data that can be reached from multiple threads. A lock for a non threadsafe object is only useful for causing deadlock.

Since your object isn't threadsafe, all you need is a member boolean PRBool mBusy, which you set to true while you're busy and check when someone asks you to do work. If you're busy, throw an nsresult exception.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

On second thought, no.  We *should not* be locking on the main thread in non-threadsafe interfaces.  For whatever reason this was added, we should revisit the issue, but locking *is not* the right solution.

This patch removes the lock entirely.  The test is still valid of course.

Comment 5

[Asaf Romano (gone)]

So, as i told you over irc, this caused various crashes on shutdown, probably due to livemark refresh. We should revise this issue before reverting the lock.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 302625 [details] [diff] [review]
v2.0

so, r=mano as long as you make sure to test that this crash no longer happens.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> (From update of attachment 302625 [details] [diff] [review])
> so, r=mano as long as you make sure to test that this crash no longer happens.
bug 382073 comment 20 for my own sanity.  I'm still not totally sure I understand what causes the crash to make sure it doesn't happen now though...

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v2.0.1

unbitrot.  I submitted this to the try server so for those interested in helping to test this can.

Comment 9

[Shawn Wilsher :sdwilsh]

tryserver builds showing up here:
https://build.mozilla.org/tryserver-builds/2008-02-21_11:[e-mail]-405497/

Comment 10

[Shawn Wilsher :sdwilsh]

So, playing with livemarks, I'm not seeing the crash.  Of course, I don't see steps to reproduce that crash anywhere, so it's hard to say that it went away.

Comment 11

[Shawn Wilsher :sdwilsh]

Checking in browser/components/nsBrowserGlue.js;
new revision: 1.54; previous revision: 1.53
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
new revision: 1.153; previous revision: 1.152
Checking in toolkit/components/places/src/nsNavBookmarks.h;
new revision: 1.52; previous revision: 1.51
Checking in toolkit/components/places/src/nsNavHistory.cpp;
new revision: 1.256; previous revision: 1.255
Checking in toolkit/components/places/src/nsNavHistory.h;
new revision: 1.140; previous revision: 1.139
Checking in toolkit/components/places/tests/unit/test_405497.js;
initial revision: 1.1

Comment 12

[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