Created attachment 346000 [details] [diff] [review] Like so We get a signature verifier on the off chance that the jar is signed. We should hold off on that until we actually know that it is. Chrome jars, notably, are not. Patch attached. mstoltz is kinda awol, so I guess I'll go with dveditz for the review; no really good reviewers here.
9 years ago
We should check this into old branches, too. We originally ran across this problem when an XMLHttpRequest security fix coupled with the FoxyProxy addon caused a top-crash (bug 456705). FoxyProxy worked around the problem, but it's a pattern copied by/from other addons and the crash is still there, though at lower levels (FoxyProxy was the popular one). And the workaround--make FoxyProxy use "flat" chrome rather than jars--is pretty ugly.
Daniel, do you think I should try to get this into b2?
I think we should try to get this in b2
One easy way to verify this fix is to go back to an earlier version of foxyproxy and see that it doesn't cause the https crashes.
Comment on attachment 346000 [details] [diff] [review] Like so OK, trying for approval. This should be a very safe patch, and we want it on branches.
Boris, does this patch apply on 1.9.0 and 1.8.1? If so, can you request approval on it? We can take this before it lands on trunk (though it's not preferred).
Comment on attachment 346000 [details] [diff] [review] Like so This applies to 1.9 branch (with fuzz=3 because of mMtime not being there on 1.9 branch).
Comment on attachment 346000 [details] [diff] [review] Like so Approved for 18.104.22.168, a=dveditz for release-drivers
Created attachment 348232 [details] [diff] [review] 1.8 branch merge
Comment on attachment 348232 [details] [diff] [review] 1.8 branch merge Approved for 22.214.171.124, a=dveditz for release-drivers
Landed on both branches.
Comment on attachment 346000 [details] [diff] [review] Like so a=beltzner for 1.9.1b2
fwiw, it looks like this patch dropped Ts by ~50ms on Windows (~4%), ~100ms on Mac (~7%), and maybe ~25ms on Linux (~1.8%), though that's harder to tell because Windows is a little more sporadic right now. Wins all around!
Pushed changeset 0c3300f451d6 to m-c. Can't think of a way to test this, really, other than Ts.
This change ostensibly is the cause of Bug 465974 Crash during start of firefox with weave installed I wonder how many other uses of PSM have historically relied on it being already initialized by the time they use it, and will now break.
Weave isn't using PSM. It's using NSS directly without doing anything to make sure it's initialized. I'd hope that there are very few binary extensions (already a given, btw) that do silly things like that.
Paul, might be a good idea to document this somewhere. In case anyone is using NSS like Weave is.
a quick scan: http://mxr-test.konigsberg.mozilla.org/addons/search?string=nss_&find=dll&filter=.&hitlimit=1 shows two likely candidates: 4522 and 4471 http://mxr-test.konigsberg.mozilla.org/addons/search?string=nss_&find=%5C.so&filter=.&hitlimit=1 shows three candidates including allpeers (sanity check): 3234, 4522 and 4471 http://mxr-test.konigsberg.mozilla.org/addons/search?string=nss_&find=dylib&filter=.&hitlimit=1 shows two likely candidates: 4522 and 4471
Is there any way to verify this change for 126.96.36.199 and 188.8.131.52? I see that there is no automated testcase (nor manual one).
Not easily, no. I verified by setting breakpoints in a debugger and seeing whether browser startup hit them. This _did_ cause a Ts drop on 1.9.0 talos machines, if that counts.
Perf drops are a great way to validate fixes! :-)
(In reply to comment #20) > Is there any way to verify this change for 184.108.40.206 and 220.127.116.11? Can't you go back to bug 456705? Get the pre-workaround crashing FoxyProxy (version 2.8.5) and verify it doesn't crash anymore when loading an https page. Or did some other fix in the meantime also stop the crash? At least to verify in 18.104.22.168; not sure 3.0.x crashed in that case.
I've verified that bug 456705 is fixed with 22.214.171.124 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:126.96.36.199) Gecko/2008120316 Firefox/188.8.131.52. It definitely reproduces with 184.108.40.206.
There is something with this patch 0c3300f451d6 that interacts badly with Flashblock and SSL sites on trunk. I earlier posted bug 470941 about a crash on loading SSL sites, and bisect pointed to the patch here.
In reply to comment 25, I think there is probably a lot of code that doesn't check to be sure that PSM is initialized before using SSL. Instead, it just assumes that PSM has been initialized. In the past, the probability that PSM was initialized was higher. Now, it's lower, and we're seeing bugs in software that fails to initialize it. I view this as a case where latent bugs in code that fails to ensure initialization have been exposed by a valid change.
Adding some thoughts. Any code attempting to use NSS directly at least must trigger NSS init. Requesting any service offered by PSM is sufficient, as PSM will automatically init NSS at that time. If PSM service creation fails, then don't use NSS. For example, getService(nsISecretDecoderRing). There is another potential problem, runtime NSS re-init. SeaMonkey's has a feature "tools / switch profile" which requires to shut down NSS and init NSS again in the new profile directory. However, Firefox' private browsing mode may be another scenario where the same approach is required. As of today, notification events are used to have PSM control the init/shutdown state of NSS. If code outside of PSM has a desire to use NSS directly, it must listen to those events, too. It must avoid calling NSS code while NSS is shut down. Furthermore, code outside of PSM must not hold on to resource handles owned by NSS. Or that code must listen to the events and free resources accordingly. If code outside of PSM holds on to NSS resources directly, this can prevent NSS shutdown/re-init to function, and therefore disable features like profile switching or maybe switching to/from private browsing mode.
The Mozilla platform no longer supports profile switching during runtime. Private browsing does not shut down PSM either. So I think that in our current platform PSM/NSS is init-once and stays initialized until shutdown.
> Private browsing does not shut down PSM either. It'll need to to fix the bug about it caching user certificates across the switch to private browsing mode, if we plan to fix it.
(In reply to comment #28) > The Mozilla platform no longer supports profile switching during runtime. > > Private browsing does not shut down PSM either. So I think that in our current > platform PSM/NSS is init-once and stays initialized until shutdown. But it might be necessary to change that. If a user owns personal certificates, then there is a risk of unintentional SSL client authentication in private browsing mode. The cleanest solution I'm aware of is a shutdown and re-init of NSS with an empty cert database. See my comment in bug 463256 comment 31.
(In reply to comment #28) > The Mozilla platform no longer supports profile switching during runtime. Do you mean that Firefox no longer supports it? Or do you mean that the underlying "platform" code (common to seamonkey also) no longer supports it? Seamonkey has UI to do this. Does it now fail?
> Or do you mean that the underlying "platform" code (common to seamonkey also) > no longer supports it? The underlying code no longer supports it. > Seamonkey has UI to do this. Does it now fail? AFAIK on trunk we fake it by doing a restart.