In prep for bug 372107 I need to make the docshell and securebrowserui implement non-threadsafe nsISupports. To do that we need some changes in PSM. Proxies can't be used to touch the docshell any longer because proxies aren't safe for objects that don't implement threadsafe addref/release. All the code that touches the docshell has thus been moved to the main thread via a custom runnable. Additionally, three threads have been touching several members in nsNSSSocketInfo for a while without protection, so I added a lock to protect the ones I know about. The function that sets those members now uses recursion in the event that the socket thread modifies the callbacks while the psm thread is waiting on the main thread.

marking wanted1.9.1? to get this in the triage queue. If this needs to be blocking1.9.1?, please mark it as so.

Comment on attachment 322992 [details] [diff] [review] Patch, v1 >+ // Use this to test identity back inside the lock. >+ nsCOMPtr<nsISupports> originalCallbacks(do_QueryInterface(callbacks)); >+ >+ { >+ nsAutoLock lock(mCallbacksLock); >+ >+ nsCOMPtr<nsISupports> currentCallbacks(do_QueryInterface(mCallbacks)); You appear to be calling QI from within a lock, which is bad mojo unless you know the concrete type of mCallbacks

Ok, this maintains nsISupports instead of nsIInterfaceRequestor so that the QI isn't required inside the lock.

Comment on attachment 323948 [details] [diff] [review] Patch, v2 Yeah, I think this is good enough... it's still painful to read though.

It would be better if nsNSSDependentStuffRunnable::Run set all out vars to the derived variables, instead of expecting the caller had passed in good defaults. if (!secureUI), I think *mExternalReporting should be set to false and *mPreviousCert to null. I assume mainThread->Dispatch(runnable, NS_DISPATCH_SYNC) will block until runnable is finished, so I conclude its fine to pass pointers to your local stack variables into the runnable. Your idea to use recursion in EnsureDocShellDependentStuffKnown is smart, but I wonder if you should ensure that all local variables/references are releases before you call self in order to retry. This would call for an additional block {} around all your code, except the recursive call to self. And if you really did this, you could use a simple loop as well. I think I'd prefer a loop over recursion (stack space is precious).

May I ask you to please rename nsNSSDependentStuffRunnable ? The code you are changing is PSM (the glue between Mozilla and NSS). The stuff we are deriving here is not depending on NSS, but it's something that PSM wants. What about nsGatherDocshellInfoForPSMRunnable ? (with or without the word Gather)

Comment on attachment 323948 [details] [diff] [review] Patch, v2 I think this patch is quite good already. Can you please consider to address some of my requests from comment 5 and 6?

I wonder how this new code relates to bug 420187 and bug 428749.

(In reply to comment #5) > It would be better if nsNSSDependentStuffRunnable::Run set all out vars to the > derived variables, instead of expecting the caller had passed in good defaults. Ok, although I can't imagine we'd ever have another caller... > if (!secureUI), I think *mExternalReporting should be set to false and > *mPreviousCert to null. Are you sure? I took care to exactly emulate the current behavior which does not alter mExternalErrorReporting or mPreviousCert. I wondered about it but decided that I didn't know enough about the architecture to change that behavior. > I assume mainThread->Dispatch(runnable, NS_DISPATCH_SYNC) will block until > runnable is finished, so I conclude its fine to pass pointers to your local > stack variables into the runnable. Correct. > I think I'd prefer a loop over recursion (stack space is precious). Ok, I can make that change. (In reply to comment #6) > What about nsGatherDocshellInfoForPSMRunnable ? Will do. (In reply to comment #8) > I wonder how this new code relates to bug 420187 and bug 428749. It won't fix bug 428749 - we should probably fix that by making the threads communicate via events rather than locking.

Patch with Kai's comments addressed.

Please consider to be more reviewer friendly in the future, by attaching an ignore-whitespace version of your patches in addition (attach both the real patch and the whitespace-ignore patch). Luckily both your patches still applied and I was able to produce such diffs myself, which allowed me to look at the interdiff between your versions and figure out your exact changes between patch v2 and patch v3. You agreed to change the function to no longer use recursion, but I think you still do! You still have a recursion to EnsureDocShellDependentStuffKnown, simply wrapped it in a do/while loop, which does not help to prevent recursion. To clarify: Please do not call EnsureDocShellDependentStuffKnown from EnsureDocShellDependentStuffKnown. Maybe you simply forgot to remove that recursive call to EnsureDocShellDependentStuffKnown. Optional nit: I would have prefered to play save and init externalReporting in the caller's code, too. I apologize for the 10 weeks delay. Can you please send me email when you have a new patch revision, then I will look at your new patch quickly. Thanks.

Here's an updated patch. You were correct, I totally forgot to remove the recursive call after using the loop :( Optional nit addressed too.

Whitespace-ignore version of v4

Comment on attachment 337917 [details] [diff] [review] Patch, v4 r=kaie Thanks! I have one more concern, but we can think about it in a separate step. I wonder if we could ever end up in an endless loop, where someone constantly calls SetNotificationCallbacks, and we never break out of the loop. Maybe the loop should give up after a couple of times.

Awesome, pushed changeset 38140c8a8327 to mozilla-central. Kai, is your last point something you want a followup bug filed on?

I just backed this out to see if it fixes the sporadic Rlk in bug 454896, on behalf of sheriff rob_strong. The backout is changeset 713e17a25165. (with a merge in changeset 8e25f600225f)

Backing this out did indeed seem to fix the Rlk leak, as described in bug 454896 comment 4. So, it looks like there's a leak hiding in this bug's patch somewhere.

I think I only reviewed the latest changes, but did not review the earlier parts of the patch thorougly enough. + nsCOMPtr<nsIX509Cert> previousCert; + + nsCOMPtr<nsIRunnable> runnable = + new nsGatherDocshellInfoForPSMRunnable(callbacks, &externalReporting, + getter_AddRefs(previousCert)); This part seems confusing to me, the 3rd parameter. You must pass in a nsIX509Cert** to receive the delayed output parameter, but you use getter_AddRefs which is usually used with immediate output parameters. I don't know if this code is responsible for the leak, but I request this gets changed into less ambiguous code. nsIX509Cert *outputPreviousCert = 0; nsCOMPtr<nsIRunnable> runnable = new nsGatherDocshellInfoForPSMRunnable(callbacks, &externalReporting, &outputPreviousCert);

> Kai, is your last point something you want a followup bug filed on? As we have a need for a new patch, my minor request from comment 16 should get addressed in the next patch.

getter_AddRefs is the standard way to get an out-parameter (nsIInterface**) into a COMPtr; I don't understand comment 20 Perhaps you were thinking of dont_AddRef/already_AddRefed, which is used with direct return values?

Kai: bent and jst asked me about whether this is possible to fix for FF4.0. Thoughts? I don't have time to work on it this week but I might be able to take it later.

Comment on attachment 337917 [details] [diff] [review] Patch, v4 Review of attachment 337917 [details] [diff] [review]: ----------------------------------------------------------------- bent: The PSM chages for this (including the PSM part of bug 675221) landed a while ago. You can drop the nsNSSIOLayer.[ch] changes in your patch completely and land the rest after rebasing it. In particular, Patrick added a new, important, comment in nsHttpConnection::GetInterface about a race, and that comment must be preserved. r=bsmith with the above changes.

Is this bug still relevant? What's the status here?

(Note to self for the next time I look at this bug: afaict, only the changes in nsHttpChannel.cpp from attachment 337917 [details] [diff] [review] are still relevant.)

Actually, it looks like bug 615342 will resolve the remaining issue.