If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Rewrite: Upgrade necko to new synchronization API

RESOLVED DUPLICATE of bug 645263

Status

()

Core
Networking
RESOLVED DUPLICATE of bug 645263
9 years ago
7 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 377603 [details] [diff] [review]
v1
Attachment #377603 - Flags: review?(cbiesinger)
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).
Attachment #377603 - Flags: review?(cbiesinger) → review?(jduell.mcbugs)
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 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?
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 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!
Attachment #377603 - Flags: review?(jduell.mcbugs) → review-
Chris--we still want to do this some time, I assume?
Yep, in fact I'll probably get back to this later today or tomorrow.
Blocks: 594666
Created attachment 504533 [details] [diff] [review]
Convert necko to new synchronization primitives.
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.
Attachment #504533 - Flags: review?(jduell.mcbugs)
Attachment #504533 - Flags: feedback?(jones.chris.g)
Comment on attachment 504533 [details] [diff] [review]
Convert necko to new synchronization primitives.

Hmm, this doesn't quite work.
Attachment #504533 - Attachment is obsolete: true
Attachment #504533 - Flags: review?(jduell.mcbugs)
Attachment #504533 - Flags: feedback?(jones.chris.g)
Created attachment 510115 [details] [diff] [review]
Convert necko to new synchronization primitives.
Comment on attachment 510115 [details] [diff] [review]
Convert necko to new synchronization primitives.

This version is better.
Attachment #510115 - Flags: review?(jduell.mcbugs)
Attachment #510115 - Flags: feedback?(jones.chris.g)
Bah, somebody added something in the last two weeks that trips the deadlock detector on shutdown with this patch.
Created attachment 510141 [details] [diff] [review]
Fix nsCacheService to use a CondVar for notifying.
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.
Attachment #510141 - Flags: review?(bjarne)
Attachment #510141 - Flags: feedback?(jones.chris.g)
Comment on attachment 510115 [details] [diff] [review]
Convert necko to new synchronization primitives.

Usage of API looks fine.
Attachment #510115 - Flags: feedback?(jones.chris.g) → feedback+
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.
Attachment #510141 - Flags: feedback?(jones.chris.g) → feedback+

Comment 19

7 years ago
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?
It did on the 6th.  I'll repush tonight though, Necko seems to change often enough to mess with me :-)

Comment 21

7 years ago
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...
cjones: Is there a reason CondVar says not to use it?  I thought we preferred Mutex + CondVar to Monitors?
You're correct, the comments need to be updated.  Bug 556214 mostly covers that.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 645263
Attachment #510115 - Flags: review?(jduell.mcbugs)
Attachment #510141 - Flags: review?(bjarne)
You need to log in before you can comment on or make changes to this bug.