[FIX]Stop initializing PSM on startup when dealing with chrome jars

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({fixed1.9.0.5, perf, verified1.8.1.19})

Trunk
x86
Mac OS X
fixed1.9.0.5, perf, verified1.8.1.19
Points:
---
Bug Flags:
blocking1.9.0.5 +
wanted1.9.0.x +
blocking1.8.1.19 +
wanted1.8.1.x +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Attachment #346000 - Flags: superreview?(vladimir)
Attachment #346000 - Flags: review?(dveditz)
Attachment #346000 - Flags: superreview?(vladimir) → superreview+
Summary: Stop initializing PSM on startup when dealing with chrome jars → [FIX]Stop initializing PSM on startup when dealing with chrome jars
Attachment #346000 - Flags: review?(dveditz) → review+
Comment on attachment 346000 [details] [diff] [review]
Like so

looks good, r=dveditz
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.
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.5?
Flags: blocking1.8.1.19?
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.
Flags: blocking1.9.0.5?
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.19?
Flags: blocking1.8.1.19+
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.
Attachment #346000 - Flags: approval1.9.1b2?
Whiteboard: [needs trunk landing?]
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).
Attachment #346000 - Flags: approval1.9.0.5?
Comment on attachment 346000 [details] [diff] [review]
Like so

Approved for 1.9.0.5, a=dveditz for release-drivers
Attachment #346000 - Flags: approval1.9.0.5? → approval1.9.0.5+
Created attachment 348232 [details] [diff] [review]
1.8 branch merge
Attachment #348232 - Flags: approval1.8.1.19?
Comment on attachment 348232 [details] [diff] [review]
1.8 branch merge

Approved for 1.8.1.19, a=dveditz for release-drivers
Attachment #348232 - Flags: approval1.8.1.19? → approval1.8.1.19+
Landed on both branches.
Keywords: fixed1.8.1.19, fixed1.9.0.5
Whiteboard: [needs trunk landing?]
Comment on attachment 346000 [details] [diff] [review]
Like so

a=beltzner for 1.9.1b2
Attachment #346000 - Flags: approval1.9.1b2? → approval1.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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Depends on: 465493

Updated

9 years ago
No longer depends on: 465493
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.

Updated

9 years ago
Blocks: 466237
No longer blocks: 466237

Comment 19

9 years ago
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 1.9.0.5 and 1.8.1.19? 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 1.9.0.5 and 1.8.1.19?

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 2.0.0.19; not sure 3.0.x crashed in that case.
I've verified that bug 456705 is fixed with 1.8.1.19 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.19) Gecko/2008120316 Firefox/2.0.0.19. It definitely reproduces with 1.8.1.18.
Keywords: fixed1.8.1.19 → verified1.8.1.19

Comment 25

9 years ago
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.
Depends on: 470941
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.

Comment 27

9 years ago
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.

Comment 30

9 years ago
(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?

Comment 32

9 years ago
> 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.
You need to log in before you can comment on or make changes to this bug.