Closed Bug 405497 Opened 15 years ago Closed 15 years ago

Calling RunInBatchedMode within a RunBatched implementation hangs

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta4

People

(Reporter: asaf, Assigned: sdwilsh)

Details

Attachments

(1 file, 2 obsolete files)

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 :(
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11
Assignee: nobody → sdwilsh
Flags: in-litmus-
Whiteboard: [needs patch]
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Version: unspecified → Trunk
Attached patch v1.0 (obsolete) — Splinter Review
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.
Attachment #302316 - Flags: review?(mano)
Whiteboard: [needs patch] → [has patch][needs review Mano]
<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...
 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.
Attached patch v2.0 (obsolete) — Splinter Review
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.
Attachment #302316 - Attachment is obsolete: true
Attachment #302625 - Flags: review?(mano)
Attachment #302316 - Flags: review?(mano)
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 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.
Attachment #302625 - Flags: review?(mano) → review+
(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...

Whiteboard: [has patch][needs review Mano] → [has patch][has review][needs to be checked]
Attached patch v2.0.1Splinter Review
unbitrot.  I submitted this to the try server so for those interested in helping to test this can.
Attachment #302625 - Attachment is obsolete: true
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.
Whiteboard: [has patch][has review][needs to be checked] → [has patch][has review][can land]
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
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Keywords: qawanted
Resolution: --- → FIXED
Whiteboard: [has patch][has review][can land]
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.