679036 - Do all certificate error processing on the main thread

Status: RESOLVED DUPLICATE of bug 679140

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Process certificate errors on the main thread

Currently, we use many XPCOM proxies in the processing that follows certificate validation failing (checking for certificate error override, reporting errors, etc.). Instead of replacing each XPCOM proxy usage with a separate runnable that gets sync-dispatched to the main thread, let's move all of this processing to a single runnable that gets sync-dispatched to the main thread. I will follow up with a similar patch for the SSL error case in another bug.

Comment 1

[Luke Wagner [:luke]]

Awesome.  Will this subsume bug 678440 and bug 678547?  Also, will this replace all the recently-added runnables (in theory fixing 678710)?

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

There will be more patches that, combined with this one, fix bug 678440. This should subsume bug 678547.

I believe that the use of sync dispatch instead of sync proxies is somehow leading to bug 678710. I think I've observed it (crashes like bug 678710) with this patch too, as it is still using Sync dispatch (just fewer of them).

Comment 3

[Benjamin Smedberg]

Note that sync dispatch spins a nested event loop, which often leads to weird behavior. See the monitor class I introduced in https://bugzilla.mozilla.org/attachment.cgi?id=551776&action=edit which does what you'd expect, blocking the calling thread but not spinning a nested event loop.

Comment 4

[Luke Wagner [:luke]]

Wow, bsmedberg; that subsumes, like, everything.  Do you have any idea when that will land?  Presumably that monitor fixes these crashes we're having with the naive proxy-to-runnable transformation.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Process certificate errors on the main thread (v2)

This uses an edited version of bsmedberg's SyncRunnableBase class to do the dispatching to the main thread, instead of using regular sync dispatch.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I have found that there is a race, especially during shutdown and maybe other times. 

Background: If we don't have callbacks regustered that can give us an nsIDocShell, we will display an unhelpful dialog box and wait for the user to dismiss it. We also allow the callbacks to tell us to avoid showing this dialog box, if they provide a specific nsIBadCertHandler2 interface.

With my patch, when we detect a cert error, we dispatch an event to the main thread to handle (amongst other things) the UI notifications. However, there may be other events preceding that event in the main thread's event queue, which may result in the socket's callbacks being reset, so that we can no longer obtain the nsIDocShell and/or nsIBadCertHandler to gracefully handle the error without showing the dialog box. This is wrong, especially during shutdown.

I think that this is a problem even with the current implementation too, but the race is much less likely because the window for other events to be handled on the main thread during cert error processing is much smaller.

I propose that we change the default behavior to be to not show any UI, and require that the callbacks' nsIBadCertHandler implementation (if any) be responsible for showing any UI.

Comment 7

[:aceman]

Will this mean any functional changes to certificate and overrides processing? Or are this just code changes?

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I did find that the race condition in comment 6 can appear even with the current code and filed bug 682329 about it.

Aceman, other than not showing the dialog box by default, there would be no functional changes.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This change cannot be de-tangled from the change in bug 679140. Bug 679140 is more general so I am marking this a dup of that.