881761 - NSS for WebRTC in content process

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Since WebRTC uses NSS in network operations, we need to use NSS functions in content process when WebRTC is made e10s for B2G.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I only know http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp?from=PeerConnectionImpl.cpp#l567 uses NSS functions in content process, do you know if there are any other place that uses NSS in content ?

Comment 2

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #1)
> I only know
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> peerconnection/PeerConnectionImpl.cpp?from=PeerConnectionImpl.cpp#l567 uses
> NSS functions in content process, do you know if there are any other place
> that uses NSS in content ?

Nothing in Gecko is using NSS in content processes, AFAIK. WebRTC would become the first thing to use it in a content process.

Comment 3

[Eric Rescorla (:ekr)]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #1)
> I only know
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> peerconnection/PeerConnectionImpl.cpp?from=PeerConnectionImpl.cpp#l567 uses
> NSS functions in content process, do you know if there are any other place
> that uses NSS in content ?

See also media/mtransport/...

This is WebRTC too.

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Initialize NSS in content process when initializing peer connection

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Initialize NSS in content process when initializing peer connection v2

Still need to test with e10s udp socket.

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 779034 [details] [diff] [review]
Patch: Initialize NSS in content process when initializing peer connection v2

Review of attachment 779034 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +568,1 @@
>    // This code interferes with the C++ unit test startup code.

Why don't we want to do do this in Gonk master process if someone turns off E10S? Perhaps we should instead
remove the #ifdef and have this clause in an ordinary runtime else?

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 779034 [details] [diff] [review]
Patch: Initialize NSS in content process when initializing peer connection v2

Review of attachment 779034 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +568,1 @@
>    // This code interferes with the C++ unit test startup code.

Why don't we want to do do this in Gonk master process if someone turns off E10S? Perhaps we should instead
remove the #ifdef and have this clause in an ordinary runtime else?

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Initialize NSS in content process when initializing peer connection v3

I missed that, thanks for catching that. I keep the #ifdef so the NSS_NoDB_Init is called only when it is running in Gonk build and is in content process, to minimize the effect on non-Gonk build.

Comment 9

[Eric Rescorla (:ekr)]

Comment on attachment 786784 [details] [diff] [review]
Patch: Initialize NSS in content process when initializing peer connection v3

Review of attachment 786784 [details] [diff] [review]:
-----------------------------------------------------------------

This LGTM but please get review from bsmith as well.

Comment 10

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

Comment on attachment 786784 [details] [diff] [review]
Patch: Initialize NSS in content process when initializing peer connection v3

Review of attachment 786784 [details] [diff] [review]:
-----------------------------------------------------------------

I am just about out the door for PTO and I ran out of time to review this closely enough to give it an r- or r+. I noted a couple of obvious issues below. I will look a this when I get back if it still needs review.

If somebody needs to check this in while I am on PTO, then the reviewer should verify that none of the other NSS initialization logic in nsNSSComponent.cpp needs to be duplicated here.

Also, Without calling NSS_Shutdown() NSS is going to leak memory and we won't be able to see through leakstats what are "real" leaks versus these leaks that are only caused by not calling NSS_Shutdown. Check with bsmedberg or jlebar or bent to see if that is considered reasonable.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +559,5 @@
> +    nsCOMPtr<nsISupports> nssDummy = do_GetService("@mozilla.org/psm;1", &res);
> +    NS_ENSURE_SUCCESS(res, res);
> +  }
> +#ifdef MOZ_WIDGET_GONK
> +  else {

I am a little nervous about calling NSS_NoDB_Init more than once in the lifetime of a process. It looks like nss_Init does attempt to guard against race conditions but it isn't immediately clear to me whether it is completely safe.

@@ +564,5 @@
> +    if (NSS_NoDB_Init(nullptr) != SECSuccess) {
> +      CSFLogError(logTag, "NSS_NoDB_Init failed.");
> +      return NS_ERROR_FAILURE;
> +    }
> +    if (NSS_SetDomesticPolicy() != SECSuccess) {

If you are calling this to set the cipher suite policy, then that probably means you need to configure the cipher suites to match the preferences as seen in nsNSSComponent.cpp's CipherPrefs variable. I think it would be strange for these preferences to affect WebRTC in single-process mode but not multi-process mode, but it may be reasonable to fix that in a follow-up bug.

Comment 11

[Eric Rescorla (:ekr)]

David,

Sid suggested you could take a look at this in bsmith's absence.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I don't really have anything to add, other than there probably will be cipher suite mismatches if what's in https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#1209 and onward isn't duplicated here or refactored somehow. It would help to know exactly which NSS functions are being used by WebRTC (I assume it's more than just PK11_GenerateRandom?)

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

We'll also need to enable NSS in content process on Firefox because I add an ipc mochitest case for PeerConnection in bug 869869.

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Initialize NSS in content process when initializing peer connection v4

Based on comment 13, remove ifdef MOZ_WIDGET_GONK to enable NSS for content in desktop for test case.

Comment 15

[Eric Rescorla (:ekr)]

(In reply to David Keeler (:keeler) from comment #12)
> I don't really have anything to add, other than there probably will be
> cipher suite mismatches if what's in
> https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> nsNSSComponent.cpp#1209 and onward isn't duplicated here or refactored
> somehow. It would help to know exactly which NSS functions are being used by
> WebRTC (I assume it's more than just PK11_GenerateRandom?)

We're using all of libssl.

keeler, we configure a lot of this stuff ourselves, but we inherit the
cipher suite settings. Could you please work with kk1fff to replicate/refactor
the cipher suite code here?

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

kk1fff: I would refactor the code that sets the cipher suites into a static utility function that can be called by both nsNSSComponent::InitializeNSS and PeerConnectionImpl::Initialize. It should be pretty straightforward after bug 733644 lands.

Comment 17

[Eric Rescorla (:ekr)]

Comment on attachment 798368 [details] [diff] [review]
Patch: Initialize NSS in content process when initializing peer connection v4

Review of attachment 798368 [details] [diff] [review]:
-----------------------------------------------------------------

Patrick. Please address :keeler's comments and resubmit

Comment 18

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Thanks, David. I will update the patch.

Comment 19

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Part 1: Move cipher setting code to a static function.

Based on work of bug 733644. Moved cipher setting and observer of "security." to a static function so other function can call.

Comment 20

[u459114]

Comment on attachment 802213 [details] [diff] [review]
WIP: Part 1: Move cipher setting code to a static function.

Review of attachment 802213 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSComponent.h
@@ +140,5 @@
>    NS_METHOD Init();
>  
>    static nsresult GetNewPrompter(nsIPrompt ** result);
>    static nsresult ShowAlertWithConstructedString(const nsString & message);
> +  static nsresult InitializeCipher(nsNSSComponent *nssComponent);

I don't understand why we need to delare this function as static.
Relative betwwen mChiperSettingsObserver & its holder, nssComponent, is 1-to-1.

@@ +198,4 @@
>    void DoProfileChangeTeardown(nsISupports* aSubject);
>    void DoProfileBeforeChange(nsISupports* aSubject);
>    void DoProfileChangeNetRestore();
>    

Please help to remove this trailer space

@@ +199,5 @@
>    void DoProfileBeforeChange(nsISupports* aSubject);
>    void DoProfileChangeNetRestore();
>    
>    Mutex mutex;
>    

Please help to remove this trailer space

@@ +202,5 @@
>    Mutex mutex;
>    
>    nsCOMPtr<nsIStringBundle> mPIPNSSBundle;
>    nsCOMPtr<nsIStringBundle> mNSSErrorsBundle;
> +  nsCOMPtr<nsIObserver> mCipherSettingsOberver;

Missspell mCipherSettingsObserver

Comment 21

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to C.J. Ku[:CJKu] from comment #20)
> > +  static nsresult InitializeCipher(nsNSSComponent *nssComponent);
> 
> I don't understand why we need to delare this function as static.
> Relative betwwen mChiperSettingsObserver & its holder, nssComponent, is
> 1-to-1.

InitializeCipher() and CipherSettingObserver set up the the cipher according to the preference, the purpose of this patch is to make this piece of code able to be invoked by PeerConnectionImpl, so content and chrome could use the same setting for cipher. Making nssComponent hold mCipherSettingsOberver is to make nssComponent able to control the oberver, which should be unregistered when nss shutdown.

In the chrome process, nsNSSComponent lives as a service, but we don't initialize the service in content process, just to keep the cipher suite settings the same in content and chrome.

However, I think I should make the CipherSettingObserver singlton to prevent it from registering twice in a process.

> > +  nsCOMPtr<nsIObserver> mCipherSettingsOberver;
> 
> Missspell mCipherSettingsObserver

Thanks for catching this.

Comment 22

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move cipher setting code to a static function.

Add a function InitializeCipherSuite in mozilla::psm to make code in other module able to initialize cipher suite. The preference observer is moved CipherSettingsObserver in nsNSSComponent.cpp, to make settings change with preference without creating an nsNSSComponent instance.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 802998 [details] [diff] [review]
Part 1: Move cipher setting code to a static function.

Review of attachment 802998 [details] [diff] [review]:
-----------------------------------------------------------------

I took comment 15 to mean that just the cipher suite settings would need to be shared (i.e. https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#1230 to #1253 (the calls to SSL_CipherPrefSetDefault, SEC_PKCS12EnableCipher, and SEC_PKCS12SetPreferredCipher)). If more than that needs to be shared, I think we'll need a different approach. One idea would be to have nsNSSComponent::InitializeNSS optionally be okay with initializing without a database, in which case everything would be inherited and updated correctly.

Comment 24

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move cipher setting code to a static function.

Comment 25

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Initialize NSS in content process when initializing peer connection v5

Comment 26

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Pushed to try server to see if initializing NSS without shutdown can be seen as leak. This situation doesn't seem to be treated as leak.

https://tbpl.mozilla.org/?tree=Try&rev=716e75d87df1

Comment 27

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch to try to initialize nss in content (to see if there's any leak reported)

Comment 28

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move cipher setting code to a static function.

Move code of setting cipher suite to a static function so PeerConnectionImpl can call. I didn't call setValidationOptions() and InstallLoadableRoots(). Are these two functions also a part of cipher suite setting?

Comment 29

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Initialize NSS in content process when initializing peer connection v5

Based on tested result on try (comment 26) and conversation on IRC, remove calling NSS_Shutdown from previous patch.

Comment 30

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 808427 [details] [diff] [review]
Part 1: Move cipher setting code to a static function.

Review of attachment 808427 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - the only problem is accessing preferences off the main thread (see the comment below).
One comment, though - it looks like you're not observing changes in the cipher prefs - is this deliberate? It means any enabled/disabled ciphers will only propagate to new content processes, or after a restart.
setValidationOptions() pertains to certificate validation, and if you're not doing that, you shouldn't need to call it. Same with InstallLoadableRoots(), I think, but Brian would know for sure.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1948,5 @@
>    return rv;
>  }
> +
> +namespace mozilla {
> +namespace psm {

Include an empty line after this one.

@@ +1959,5 @@
> +
> +  bool cipherEnabled;
> +  // Now only set SSL/TLS ciphers we knew about at compile time
> +  for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
> +    cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);

Sorry I didn't catch this earlier, but my understanding is that preferences should only be accessed on the main thread. It looks like the other patch calls this function from the STS thread. If the main thread restriction doesn't apply to child processes, then this is probably fine, but otherwise we're going to have to do something else here. I would ask bsmedberg about this.

@@ +1972,5 @@
> +  SEC_PKCS12EnableCipher(PKCS12_DES_56, 1);
> +  SEC_PKCS12EnableCipher(PKCS12_DES_EDE3_168, 1);
> +  SEC_PKCS12SetPreferredCipher(PKCS12_DES_EDE3_168, 1);
> +  PORT_SetUCS2_ASCIIConversionFunction(pip_ucs2_ascii_conversion_fn);
> +}

Empty line again.

Comment 31

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Thanks for the review, David.

(In reply to David Keeler (:keeler) from comment #30)
> Comment on attachment 808427 [details] [diff] [review]
> Part 1: Move cipher setting code to a static function.
> 
> Review of attachment 808427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good - the only problem is accessing preferences off the main thread
> (see the comment below).
> One comment, though - it looks like you're not observing changes in the
> cipher prefs - is this deliberate? It means any enabled/disabled ciphers
> will only propagate to new content processes, or after a restart.

I should observe the change.

> setValidationOptions() pertains to certificate validation, and if you're not
> doing that, you shouldn't need to call it. Same with InstallLoadableRoots(),
> I think, but Brian would know for sure.
> 
> ::: security/manager/ssl/src/nsNSSComponent.cpp
> @@ +1948,5 @@
> >    return rv;
> >  }
> > +
> > +namespace mozilla {
> > +namespace psm {
> 
> Include an empty line after this one.
> 
> @@ +1959,5 @@
> > +
> > +  bool cipherEnabled;
> > +  // Now only set SSL/TLS ciphers we knew about at compile time
> > +  for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
> > +    cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);
> 
> Sorry I didn't catch this earlier, but my understanding is that preferences
> should only be accessed on the main thread. It looks like the other patch
> calls this function from the STS thread. If the main thread restriction
> doesn't apply to child processes, then this is probably fine, but otherwise
> we're going to have to do something else here. I would ask bsmedberg about
> this.

I've just asked khuey about this. It must be used in main thread, I'll post this task to main thread.

> 
> @@ +1972,5 @@
> > +  SEC_PKCS12EnableCipher(PKCS12_DES_56, 1);
> > +  SEC_PKCS12EnableCipher(PKCS12_DES_EDE3_168, 1);
> > +  SEC_PKCS12SetPreferredCipher(PKCS12_DES_EDE3_168, 1);
> > +  PORT_SetUCS2_ASCIIConversionFunction(pip_ucs2_ascii_conversion_fn);
> > +}
> 
> Empty line again.

Comment 32

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Patrick Wang [:kk1fff] from comment #31)
> I've just asked khuey about this. It must be used in main thread, I'll post
> this task to main thread.

According to gdb, this call is actually made in main thread. I'll add an assertion at the beginning of this function to make sure this function is running on main thread.

Comment 33

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move cipher setting code to a static function.

Comment 34

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Initialize NSS in content process when initializing peer connection

Comment 35

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move cipher setting code to a static function.

ClearOnShutdown asserts failure on try. Update with fix.

Comment 36

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 809854 [details] [diff] [review]
Part 1: Move cipher setting code to a static function.

Review of attachment 809854 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few changes I would like to see:
 - check that you're on the main thread every time Preferences:: is accessed (just once per function)
 - use nsresult instead of bool and check for success every time something can fail

I think this is something an official PSM peer should also have a look at (Brian is pretty busy, so maybe Honza (:mayhemer) can review it).

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +945,5 @@
> +  nsRefPtr<CipherSuiteChangeObserver> observer = sObserver.get();
> +  return observer.forget();
> +}
> +
> +bool

I think nsresult instead of bool would be best.

@@ +961,5 @@
> +void
> +CipherSuiteChangeObserver::StopObserve()
> +{
> +  if (mObserving) {
> +    Preferences::RemoveObserver(this, "security.");

It would be best to check that this succeeds, too (like you do for AddStrongObserver).

Comment 37

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move cipher setting code to a static function.

Sorry I had mixed the fix into wrong patch in previous update, that patch didn't include fix for ClearOnShutdown's assertion. The difference is that I remove ClearOnShutdown from the patch and expose StartObserve/StopObserve as static functions and let these two functions create/destroy the instance of CipherSuiteChange observer.

This update removed ClearOnShutdown and addressed comment 36. Sorry for posting wrong patch.

Honza, we are trying to provide a common interface to setup cipher suite in nss so we can call the function in content proces when it didn't need to initialize whole nss component, would you help to review this patch? Thanks.

Comment 38

[u459114]

Comment on attachment 810393 [details] [diff] [review]
Part 1: Move cipher setting code to a static function.

Review of attachment 810393 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +969,5 @@
> +    /* Look through the cipher table and set according to pref setting */
> +    bool cipherEnabled;
> +    for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
> +      if (prefName.Equals(cp->pref)) {
> +        NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::Observe can only be accessed in main thread");

Why not just put this assertion in the beginning of function?

@@ +970,5 @@
> +    bool cipherEnabled;
> +    for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
> +      if (prefName.Equals(cp->pref)) {
> +        NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::Observe can only be accessed in main thread");
> +        cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);

bool cipherEnabled =  cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);

@@ +2033,5 @@
> +
> +  bool cipherEnabled;
> +  // Now only set SSL/TLS ciphers we knew about at compile time
> +  for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
> +    cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);

bool cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);

Comment 39

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to C.J. Ku[:CJKu] from comment #38)
> > +        NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::Observe can only be accessed in main thread");
> 
> Why not just put this assertion in the beginning of function?

What I am concerning is that this function would be called many times, as long as "security." preference is changed, no matter the change makes us read the preference or not. If we assert at the beginning of this function, it would require all notifications related to "security." to be on main thread.

Comment 40

[u459114]

(In reply to Patrick Wang [:kk1fff] from comment #39)
> (In reply to C.J. Ku[:CJKu] from comment #38)
> > > +        NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::Observe can only be accessed in main thread");
> > 
> > Why not just put this assertion in the beginning of function?
> 
> What I am concerning is that this function would be called many times, as
> long as "security." preference is changed, no matter the change makes us
> read the preference or not. If we assert at the beginning of this function,
> it would require all notifications related to "security." to be on main
> thread.
IC
NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::Observe can only be accessed in main thread");
How about change this message to, for example
NS_ASSERTION(NS_IsMainThread(), "Cipher data can only be accessed in main thread")?

Comment 41

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 810393 [details] [diff] [review]
Part 1: Move cipher setting code to a static function.

Review of attachment 810393 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - just a couple final nits.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +935,5 @@
> +  NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::StartObserve() can only be accessed in main thread");
> +  if (!sObserver) {
> +    nsresult rv;
> +    sObserver = new CipherSuiteChangeObserver();
> +    if (NS_FAILED(rv = Preferences::AddStrongObserver(sObserver.get(), "security."))) {

I think it's best to separate this into two lines:

rv = Preferences...
if (NS_FAILED(rv)) ...

@@ +950,5 @@
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::StopObserve() can only be accessed in main thread");
> +  if (sObserver) {
> +    nsresult rv;
> +    if (NS_FAILED(rv = Preferences::RemoveObserver(sObserver.get(), "security."))) {

Two lines, again.

@@ +969,5 @@
> +    /* Look through the cipher table and set according to pref setting */
> +    bool cipherEnabled;
> +    for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
> +      if (prefName.Equals(cp->pref)) {
> +        NS_ASSERTION(NS_IsMainThread(), "CipherSuiteChangeObserver::Observe can only be accessed in main thread");

The observer service only runs on the main thread, so we can hoist this check to the top of this function (in theory we could remove it entirely, but I think it's good to have in case things change).

@@ +2021,5 @@
> +
> +namespace mozilla {
> +namespace psm {
> +
> +bool InitializeCipherSuite() {

I think this should also return an nsresult.

@@ +2033,5 @@
> +
> +  bool cipherEnabled;
> +  // Now only set SSL/TLS ciphers we knew about at compile time
> +  for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
> +    cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);

CJKu - I'm curious as to why this is preferable.

@@ +2049,5 @@
> +  PORT_SetUCS2_ASCIIConversionFunction(pip_ucs2_ascii_conversion_fn);
> +
> +  // Observe preference change around cipher suite setting.
> +  if (NS_FAILED(CipherSuiteChangeObserver::StartObserve())) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_ERROR, ("Unable to start to observe cipher suite preference change\n"));

As far as I can tell, if NSS hasn't been initialized like normal (as in the case of content process WebRTC), gPIPNSSLog will be null. Maybe just null-check it first? (I'm open to other solutions...)

Comment 42

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to David Keeler (:keeler) from comment #41)
> @@ +2033,5 @@
> > +
> > +  bool cipherEnabled;
> > +  // Now only set SSL/TLS ciphers we knew about at compile time
> > +  for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
> > +    cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);
> 
> CJKu - I'm curious as to why this is preferable.

That didn't work - let me try again:

(In reply to C.J. Ku[:CJKu] from comment #38)
> @@ +2033,5 @@
> > +
> > +  bool cipherEnabled;
> > +  // Now only set SSL/TLS ciphers we knew about at compile time
> > +  for (CipherPref* cp = CipherPrefs; cp->pref; ++cp) {
> > +    cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);
> 
> bool cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);

CJKu - I'm curious as to why this is preferable.

Comment 43

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to David Keeler (:keeler) from comment #42)
> > bool cipherEnabled = Preferences::GetBool(cp->pref, CIPHER_ENABLED_DEFAULT);
> 
> CJKu - I'm curious as to why this is preferable.

I suppose that because it limits the life of cipherEnablid to scope of for-loop.

Comment 44

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to C.J. Ku[:CJKu] from comment #40)
> NS_ASSERTION(NS_IsMainThread(), "Cipher data can only be accessed in main
> thread")?

I suggest to keep the detail, since there are many functions that we asserted to run on main thread. If the assertion fails, we would need the information to tell us in which function the assertion fails.

Comment 45

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move cipher setting code to a static function. (r=keeler)

Address comment 41. Removing PR_LOG in InitializeCipherSuite and making InitializeCipherSuite return the result of StartObserve, let caller handle the error code.

Also moving cipherEnabled in CipherSuiteChangeObserver::Observe to for-loop's scope.

Comment 46

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Initialize NSS in content process when initializing peer connection

Comment 47

[Eric Rescorla (:ekr)]

Comment on attachment 811873 [details] [diff] [review]
Part 2: Initialize NSS in content process when initializing peer connection

Review of attachment 811873 [details] [diff] [review]:
-----------------------------------------------------------------

This lgtm but I don't understand the setup issues well enough to know if
your PSM work is right.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +32,5 @@
>  #include "nsIConsoleService.h"
>  #include "nsThreadUtils.h"
>  #include "nsProxyRelease.h"
> +#include "nsIObserver.h"
> +#include "nsIObserverService.h"

Are these needed? I don't see them being used

Comment 48

[Honza Bambas (:mayhemer)]

Comment on attachment 811872 [details] [diff] [review]
Part 1: Move cipher setting code to a static function. (r=keeler)

Review of attachment 811872 [details] [diff] [review]:
-----------------------------------------------------------------

not locally testes and not checked byte-by-byte.

r=honzab

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +937,5 @@
> +    sObserver = new CipherSuiteChangeObserver();
> +    nsresult rv = Preferences::AddStrongObserver(sObserver.get(), "security.");
> +    if (NS_FAILED(rv)) {
> +      sObserver = nullptr;
> +      return rv;

Can you fill sObserver after successful AddStrongObserver?

@@ +953,5 @@
> +    nsresult rv = Preferences::RemoveObserver(sObserver.get(), "security.");
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    sObserver = nullptr;

Won't this unnecessarily leak?

@@ +2020,5 @@
> +
> +namespace mozilla {
> +namespace psm {
> +
> +nsresult InitializeCipherSuite() {

{ on a new line

Comment 49

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Honza Bambas (:mayhemer) from comment #48)
> @@ +953,5 @@
> > +    nsresult rv = Preferences::RemoveObserver(sObserver.get(), "security.");
> > +    if (NS_FAILED(rv)) {
> > +      return rv;
> > +    }
> > +    sObserver = nullptr;
> 
> Won't this unnecessarily leak?

Do you mean that when the observer is not in the observer service and RemoveObserver() returns failure, the sObserver won't be cleaned? I can move |sObserver = nullptr| to the line right after RemoveObserver().

What I used to think is that when we failed to remove an observer, if we need to retry somehow, we will need to keep the reference. But looking at the code inside RemoveObserver(), the failure happens only when wrong topic or wrong instance reference is supplied to RemoveObserver(), so keeping the reference doesn't seem necessary.

Comment 50

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #47)
> > +#include "nsIObserver.h"
> > +#include "nsIObserverService.h"
> 
> Are these needed? I don't see them being used

These are not necessary. It was used in a old version of patch and I forgot to remove...

Comment 51

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

Comment on attachment 811873 [details] [diff] [review]
Part 2: Initialize NSS in content process when initializing peer connection

Review of attachment 811873 [details] [diff] [review]:
-----------------------------------------------------------------

Note that this will probably result in memory leaks because NSS_Shutdown isn't called. That is OK with me if it is OK with bsmedberg. You may have to add annotations to our memory leak reporting stuff to prevent them from reporting these leaks.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +73,5 @@
> +
> +#ifdef MOZILLA_INTERNAL_API
> +static nsresult InitNSSInContent()
> +{
> +  if (XRE_GetProcessType() != GeckoProcessType_Content) {

Use MOZ_NOT_REACHED here.

@@ +77,5 @@
> +  if (XRE_GetProcessType() != GeckoProcessType_Content) {
> +    return NS_OK;
> +  }
> +
> +  static bool nssStarted = false;

This is not thread-safe. If it will always be called on the main thread then you can do:

NS_ENSURE_TRUE(NS_IsMainThread(), NS_ERROR_NOT_SAME_THREAD) beforehand.

If it will be called on different threads then I recommend using PR_CallOnce.

@@ +86,5 @@
> +  if (NSS_NoDB_Init(nullptr) != SECSuccess) {
> +    CSFLogError(logTag, "NSS_NoDB_Init failed.");
> +    return NS_ERROR_FAILURE;
> +  }
> +  if (NSS_SetDomesticPolicy() != SECSuccess) {

NSS_SetDomesticPolicy() should be called in mozilla::psm::InitializeCipherSuite. It is part of the SSL configuration and it can/should be shared.

I think you also need to disable MD5-based signatures:

  NSS_SetAlgorithmPolicy(SEC_OID_MD5,
      0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
  NSS_SetAlgorithmPolicy(SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION,
      0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
  NSS_SetAlgorithmPolicy(SEC_OID_PKCS5_PBE_WITH_MD5_AND_DES_CBC,
      0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);


This should be factored out into a shared function too, seperate from InitializeCipherSuite() because other work we're doing requires those things to be done in separate functions.

Comment 52

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #51)
>   NSS_SetAlgorithmPolicy(SEC_OID_MD5,
>       0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
>   NSS_SetAlgorithmPolicy(SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION,
>       0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
>   NSS_SetAlgorithmPolicy(SEC_OID_PKCS5_PBE_WITH_MD5_AND_DES_CBC,
>       0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
> 
> 
> This should be factored out into a shared function too, seperate from
> InitializeCipherSuite() because other work we're doing requires those things
> to be done in separate functions.

Can I move the piece of code that reads preference and configures md5 into InitializeCipherSuite(), so InitializeCipherSuite() can setup md5 according to preference?

Comment 53

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

(In reply to Patrick Wang [:kk1fff] from comment #52)
> > This should be factored out into a shared function too, seperate from
> > InitializeCipherSuite() because other work we're doing requires those things
> > to be done in separate functions.
> 
> Can I move the piece of code that reads preference and configures md5 into
> InitializeCipherSuite(), so InitializeCipherSuite() can setup md5 according
> to preference?

I have already written the separate function in a patch for another bug:

void
ConfigureMD5(bool enabled)
{
  if (enabled) { // set flags
    NSS_SetAlgorithmPolicy(SEC_OID_MD5, 
        NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE, 0);
    NSS_SetAlgorithmPolicy(SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION,
        NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE, 0);
    NSS_SetAlgorithmPolicy(SEC_OID_PKCS5_PBE_WITH_MD5_AND_DES_CBC,
        NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE, 0);
  }
  else { // clear flags
    NSS_SetAlgorithmPolicy(SEC_OID_MD5,
        0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
    NSS_SetAlgorithmPolicy(SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION,
        0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
    NSS_SetAlgorithmPolicy(SEC_OID_PKCS5_PBE_WITH_MD5_AND_DES_CBC,
        0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
  }
}

For WebRTC, you do not need to use the value of the preference. You can always call ConfigureMD5(false) so that MD5 is never enabled.

Comment 54

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #53)
> For WebRTC, you do not need to use the value of the preference. You can
> always call ConfigureMD5(false) so that MD5 is never enabled.

I see. Then I will expose the function (it's static now).

Comment 55

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Exposing configureMD5.

Comment 56

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: Initialize NSS in content process when initializing peer connection

Address comment 51.

Benjamin, would you help to comment? We are initializing nss in content process without calling NSS_shutdown. This may introduce leaks. Is this kind of leak acceptable? Thanks.

Comment 57

[Benjamin Smedberg]

Why are we using NSS in the content process? NSS is a huge memory hog, and our prior decision was to only intialize NSS in the chrome process (for networking) to avoid the memory hit. Has that decision changed?

How are you handling things like the certificate store, which the content process should not be able to access because of the sandbox?

To the small question at hand, I suspect that it really only matters for leak checking whether we shut down NSS: but leak-checking builds should certainly shut it down. Why wouldn't you?

Comment 58

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

Comment on attachment 815718 [details] [diff] [review]
Part 2: Exposing configureMD5.

Review of attachment 815718 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/PublicSSL.h
@@ +14,5 @@
>  namespace psm {
>  
>  void InitializeSSLServerCertVerificationThreads();
>  void StopSSLServerCertVerificationThreads();
> +void configureMD5(bool enabled);

s/configureMD5/ConfigureMD5/. There is a convention to start exported functions with a uppercase letters and non-exported functions with lowercase letters.

Comment 59

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #57)
> Why are we using NSS in the content process? NSS is a huge memory hog, and
> our prior decision was to only intialize NSS in the chrome process (for
> networking) to avoid the memory hit. Has that decision changed?
> 
> How are you handling things like the certificate store, which the content
> process should not be able to access because of the sandbox?

WebRTC uses NSS's DTLS and the socket that is used by NSS is implemented by ICE, which is also a part of WebRTC and also runs in content process, so we initialize NSS for the secure connection. However, certificate store is not used in WebRTC, so we didn't load that part.

> To the small question at hand, I suspect that it really only matters for
> leak checking whether we shut down NSS: but leak-checking builds should
> certainly shut it down. Why wouldn't you?

If leak-checking build would shutdown, do we still need to shut it down? If we need, do you have a suggestion about where we should put the NSS_Shutdown? I've tried observer of 'xpcom-shutdown', but the observer is never called in a content process when I test on a phone.

Comment 60

[Benjamin Smedberg]

How much memory does this form of NSS cost?

Do we even have leak-checking builds on-phone? I suspect that content processes in B2G may already shutdown via exit() or even being killed when they are no longer needed. Content processes in desktop builds I believe shut down completely (for now anyway).

Comment 61

[Eric Rescorla (:ekr)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #60)
> How much memory does this form of NSS cost?

Not sure. It's not really optional, though, so I guess we'll find out.


> Do we even have leak-checking builds on-phone? I suspect that content
> processes in B2G may already shutdown via exit() or even being killed when
> they are no longer needed. Content processes in desktop builds I believe
> shut down completely (for now anyway).
.

Comment 62

[Eric Rescorla (:ekr)]

EKR, this is a note to yourself to see if we need to do any leak checking here.

Comment 63

[Eric Rescorla (:ekr)]

Patrick,

Can you see the about:memory difference before and after NSS startup? If it's small, we can basically ignore this.

Comment 64

[Eric Rescorla (:ekr)]

Brian, can NSS be started up again after we shut it down? If not, we oviously can't try to shut it down.

Comment 65

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #63)
> Patrick,
> 
> Can you see the about:memory difference before and after NSS startup? If
> it's small, we can basically ignore this.

I built an image that loads NSS in preallocated process for experiment. Accoding to about:memory, the normal preallocated process consumes 8.71 MB (explicit allocations) and the process with NSS consumes 9.99 MB.

Comment 66

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Patrick Wang [:kk1fff] from comment #65)
> I built an image that loads NSS in preallocated process for experiment.
> Accoding to about:memory, the normal preallocated process consumes 8.71 MB
> (explicit allocations) and the process with NSS consumes 9.99 MB.

Tried again, the preallocated process with NSS consumes 8.7 MB, it looks there are some noise. I changed way of test to measure the memory usage before and after NSS initialized. The amount of explicit allocations before NSS initialized is 9.52 MB, and it is 9.41 MB after NSS initialized.

Comment 67

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move cipher setting code to a static function. (r=keeler,honzab)

Comment 68

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Exposing configureMD5. (r=bsmith)

Comment 69

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: Initialize NSS in content process when initializing peer connection(r=ekr,bsmith)

Comment 70

[Eric Rescorla (:ekr)]

In Chrome at least, they just do a single startup and never shut down
in the renderers, so I don't think we have precedent for repeated startup
and shutdown under JS control (as opposed to occasional). Based on this and
kk1fff's measurements, I propose leaving as-is.

Any objections? bsmedberg?

Comment 71

[Benjamin Smedberg]

I don't have a clear understanding of whether that extra 1.2MB is a killer or not, or what the alternatives are, so I can't provide much more guidance here.  Are the B2G product drivers aware of this and ok with it?

Comment 72

[Eric Rescorla (:ekr)]

It seems like the 1.2 MB is the outer edge and looking at c66, it's possible the answer is 0. Given that this only is incurred if we do webrtc, this seems acceptable. Asking for Faramarz's opinion.

Comment 73

[Maire Reavy [:mreavy]]

(In reply to Eric Rescorla (:ekr) from comment #72)
> It seems like the 1.2 MB is the outer edge and looking at c66, it's possible
> the answer is 0. Given that this only is incurred if we do webrtc, this
> seems acceptable. Asking for Faramarz's opinion.

Faramarz -- Doing a needinfo to get your thoughts on this since you will be driving the v1.3 release.  Thanks.

Comment 74

[Jason Smith [:jsmith]]

(In reply to Maire Reavy [:mreavy] from comment #73)
> (In reply to Eric Rescorla (:ekr) from comment #72)
> > It seems like the 1.2 MB is the outer edge and looking at c66, it's possible
> > the answer is 0. Given that this only is incurred if we do webrtc, this
> > seems acceptable. Asking for Faramarz's opinion.
> 
> Faramarz -- Doing a needinfo to get your thoughts on this since you will be
> driving the v1.3 release.  Thanks.

For getting a response quicker, you might want to just send an email over to [e-mail].

Comment 75

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

(In reply to Eric Rescorla (:ekr) from comment #64)
> Brian, can NSS be started up again after we shut it down? If not, we
> oviously can't try to shut it down.

I do not think we should bother with shutting down NSS. It has caused a lot of complexity and bugs in the normal (parent) process that isn't justified, especially in the child. Note that we can't avoid shutting down NSS in the parent process as easily as we can avoid shutting it down in the content process, because in the parent process we read/write to the certificate database, which requires clean NSS shutdown (probably). In the child process, we don't read/write to those databases so we should be fine. We just need to annotate the leaktests (if any) to ignore any NSS-related leaks.

1.2MB sounds like more memory than NSS should be using. But, I suggest we add a follow-up bug that blocks the "operation slim-fast" bug for that. I agree with EKR that 1.2MB is reasonable as long as we lazily initialize NSS in the content process only when WebRTC is needed. Please add a comment about this to the code that calls NSS_NoDB_Init before landing it.

Comment 76

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 4: Call NSS_Shutdown when xpcom is going down.

Per talked with bsmedberg on IRC, shuting NSS down should be the right way to prevent from being reported by leak checking build.

Brian, is it fine if we call NSS_shutdown in xpcom-shutdown? Based on comment 60, the content process should close directly without firing xpcom-shutdown event, this shuold only affect leak checking build.

Comment 77

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

As previous try result on comment 26, starting NSS without shutting it down doesn't cause try failure.

Comment 78

[Eric Rescorla (:ekr)]

Comment on attachment 821522 [details] [diff] [review]
Part 4: Call NSS_Shutdown when xpcom is going down.

Review of attachment 821522 [details] [diff] [review]:
-----------------------------------------------------------------

David, bsmith seems busy. Could you take a look?

Comment 79

[Eric Rescorla (:ekr)]

Part four seems like a bad idea to me. This is going to cause problems because it's asynchronous
with respect to stuff happening in the STS thread, namely DTLS and ICE packet processing which
may be going on and hasn't shut down.

bsmedberg: is there some actual test build/process that alarms without something like patch 4?
If not, I propose we omit it, especially based on c75.

Comment 80

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

FWIW, I agree with Brian in comment 75.

Comment 81

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

Comment on attachment 821522 [details] [diff] [review]
Part 4: Call NSS_Shutdown when xpcom is going down.

Review of attachment 821522 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 26 explains why this isn't needed. Comment 75 explains why it is a bad idea. EKR, dkeeler, and I all agree it is a bad idea. Bad idea is bad.

Patrick, thanks for your effort on this. Sorry there's been so much back-and-forth on this issue. But, I suggest you push the rest and just avoid shutting down NSS in the content process at all.

Comment 82

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Thanks, Brian. I will go ahead and push the 3 parts then.

Comment 83

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move cipher setting code to a static function. (r=keeler,honzab) (check-in)

Comment 84

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Exposing configureMD5. (r=bsmith) (check-in)

Comment 85

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: Initialize NSS in content process when initializing peer connection(r=ekr,bsmith) (check-in)

Comment 86

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/780d664ce03e
https://hg.mozilla.org/integration/b2g-inbound/rev/5bc57df50c31
https://hg.mozilla.org/integration/b2g-inbound/rev/b48d40d8eb5f

Comment 87

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/780d664ce03e
https://hg.mozilla.org/mozilla-central/rev/5bc57df50c31
https://hg.mozilla.org/mozilla-central/rev/b48d40d8eb5f

Comment 88

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Got a mail about performance regression on Android.

Regression: B2g-Inbound - Tp4 Shutdown Mobile - Android 4.0.4 - 14.3% increase
------------------------------------------------------------------------------
    Previous: avg 25204358.333 stddev 1339.917 of 12 runs up to revision c01c64954f59
    New     : avg 28805108.333 stddev 1575.066 of 12 runs since revision b48d40d8eb5f
    Change  : +3600750.000 (14.3% / z=2687.292)
    Graph   : http://mzl.la/17JHwJc

Comment 89

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Looking at the test report before and after last weekend, it seems running time of Tp4 Shutdown Mobile in all branches (with or without these patches) are increased.