493146 - Rewrite: Upgrade necko to new synchronization API

Status: RESOLVED DUPLICATE of bug 645263

(Core :: Networking, defect)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v1

Comment 2

[Jason Duell]

Comment on attachment 377603 [details] [diff] [review]
v1

I'll take a look first, and then we can ping Christian (he doesn't read mail from Bugzilla generally).

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, my fault.

Jason, you might want to investigate being added as a necko peer.  I don't know what this involves, but having your name on the list would be useful for people like me who only read http://www.mozilla.org/owners.html.

Comment 4

[Jason Duell]

Comment on attachment 377603 [details] [diff] [review]
v1

As per bug 493116, I think we should change all of the Mutex (etc) pointers to be just direct members.   I'm not clear on why you said this is hard to verify as safe:  from my understanding, the lock allocation is always safe (so they it can run in constructors), and any place in the code where we previously modified the pointer will barf after the transform, since you've made the Mutex = operator private.  So what's the risk?  Am I missing something?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hopefully it's not a problem.  However, "questionable" code might cast these guys to |void*| and use them as arguments to callbacks, for example.  Depending on how this is done, the C++ type system can catch the error; for some ways, it can't.  These errors can lead to bugs I don't want to fix, because they require actually understanding somewhat what the code is trying to do ;).

Basically, I just want someone familiar with the code to say, "we're not doing stupid crap like that."  In all likelihood, there shouldn't be any problems making these guys direct members.

Comment 6

[Jason Duell]

Comment on attachment 377603 [details] [diff] [review]
v1

Go ahead and run your tool to make a new patch with locks, etc., as direct members rather than pointers, and I'll look over the code to make sure there's no funny business with casting.  Thanks!

Comment 7

[Jason Duell]

Chris--we still want to do this some time, I assume?

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yep, in fact I'll probably get back to this later today or tomorrow.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Convert necko to new synchronization primitives.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 504533 [details] [diff] [review]
Convert necko to new synchronization primitives.

Without being aware of this bug, I went through and converted Necko's nsAutoLocks and PRLocks to mozilla::Mutexes.  After I found this, I finished up the rest of them.  I didn't see any funny business with casting locks while I was poking around.

I think this would make it easier for the deadlock detector to tell us what's going wrong in Bug 594666, which is one of our most frequent oranges.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 504533 [details] [diff] [review]
Convert necko to new synchronization primitives.

Hmm, this doesn't quite work.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Convert necko to new synchronization primitives.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 510115 [details] [diff] [review]
Convert necko to new synchronization primitives.

This version is better.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Bah, somebody added something in the last two weeks that trips the deadlock detector on shutdown with this patch.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Fix nsCacheService to use a CondVar for notifying.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 510141 [details] [diff] [review]
Fix nsCacheService to use a CondVar for notifying.

Bug 620660 added SyncWithCacheIOThread which blocks (the main thread?) on the cache thread while pending events are processed.  This patch changes that notification to happen through a condvar instead of a monitor and cleans up some debug assertions involving the cache service.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 510115 [details] [diff] [review]
Convert necko to new synchronization primitives.

Usage of API looks fine.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 510141 [details] [diff] [review]
Fix nsCacheService to use a CondVar for notifying.

Some things in here look a little squirrely, but that's for you and your reviewer to settle.  Don't see anything wrong with API usage.  The monitor->condvar change is OK and righteous as long as the APIs aren't re-entrant, which I can't provide feedback on.

Comment 19

[Bjarne (:bjarne)]

Comment on attachment 510141 [details] [diff] [review]
Fix nsCacheService to use a CondVar for notifying.

I'll get to this shortly, but does it pass the tryserver?

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It did on the 6th.  I'll repush tonight though, Necko seems to change often enough to mess with me :-)

Comment 21

[Bjarne (:bjarne)]

Comment on attachment 510141 [details] [diff] [review]
Fix nsCacheService to use a CondVar for notifying.

If using CondVars is the preferred mechanism these days, maybe someone should rephrase this?  :)

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/CondVar.h#52

>+#ifdef DEBUG
>+    static void      AssertOwnsLock();
>+#endif

Either use this method consistently or remove it I guess...

I'm still trying to get my head around how associating the global (and frequently used) lock with a condvar will work in an isolated case like this. I tried something similar in an early version of the patch for bug #620660, got some nasty results and abandoned the approach (also partly because of the docs mentioned above  :) ). Results from a tryserver-run might clarify this. Patch has bitrotted btw...

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

cjones: Is there a reason CondVar says not to use it?  I thought we preferred Mutex + CondVar to Monitors?

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

You're correct, the comments need to be updated.  Bug 556214 mostly covers that.