Maniac Vlad Florin (:vladmaniac) Reporter
	Description • 14 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:2.0b2pre) Gecko/20100629 Minefield/4.0b2pre
Build Identifier: 20100629041239

No green or blue "Larry" icon is shown. Both for the untrusted connections and secure ones (with SSL cert) a grey "Larry" is displayed. 

Reproducible: Always

Steps to Reproduce:
1.Navigate https://www.paypal.com
2.Click on the site's favicon
3.Inspect the applet being display correctly with no errors of any kind. 
Actual Results:  
Green security guard should appear. 

Expected Results:  
Grey security icon is displayed instead, specifically for untrusted sites, as here is not the case.

	Aakash Desai [:aakashd]
	Comment 2 • 14 years ago

This works for me on build:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b2pre) Gecko/2010630 Namoroka/4.0b2pre Fennec/2.0a1pre

both on trunk and fennec-electrolysis-maemo5-gtk builds.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 3 • 14 years ago

I still see this problem

	[:fabrice] Fabrice Desré
	Comment 4 • 14 years ago

It's still broken today, but it was working at changeset 39d29c21907c

I'll bisect from here.

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 5 • 14 years ago

I can confirm that. I was just about to close the bug to resolved:worksforme

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 6 • 14 years ago

Fabrice, can you take a look at this? We have access to a better secureBrowserUI now. See bug 536301 and bug 568502.

	[:fabrice] Fabrice Desré
	Comment 7 • 14 years ago

This log shows the activity of the nsSecureBrowserUIImpl whild loading a simple https page (https://demo.pairssl.com/)

The log shows that in GetSecurityStateFromSecurityInfo() the info parameter is null. I think this comes from the call to ExtractSecurityInfo(nsIRequest) where we fail to get securityInfo from the request's channel or from the request's implementation of nsISecurityInfoProvider

	Honza Bambas (:mayhemer) Assignee
	Comment 8 • 14 years ago

Then it looks the patch from bug 536301 stopped working.  It was exactly created to provide security info object on the content process (attach it to HttpChannelChild where from the sec UI impl is trying to extract it).

I'll re-check this with multiprocess firefox build first.

	Honza Bambas (:mayhemer) Assignee
	Comment 9 • 14 years ago

This is really bad.

We correctly send security info serialization to HttpChannelChild, but we fail to instantiate nsNSSSecurityInfo object because we do not allow nsNSSComponent (and the whole NSS) be instantiated on the child process on which nsNSSSecurityInfo depends.  This is intentional by bug 559711.

Sorry, I had to catch this much sooner, this has been broken since 2010-08-10.

Options:

1. back bug 559711 out again (simple, but I cannot say what all this will break)

2. check if nsNSSSecurityInfo and other classes necessary to determine security state are really dependent on nsNSSComponent and bypass dep check on child process (I don't think we success in this, but I will try to)

3. do a major architectural change where we have a mirror of loadgroups/docshells on the parent process and track the state there as I proposed months ago (it's clear it's way too late for this option)

	Honza Bambas (:mayhemer) Assignee
	Comment 10 • 14 years ago

- makes some PSM components instantiable on non-chrome processes
- nsNSSCertificate only keeps the cert serialization on non-chrome processes ; instantiating mCert fails because it needs NSS be initialized

	[:fabrice] Fabrice Desré
	Comment 11 • 14 years ago

Honza, I did a build with your v1 patch, but was unable to load any SSL site with it applied.

	Honza Bambas (:mayhemer) Assignee
	Comment 12 • 14 years ago

(In reply to comment #11)
> Honza, I did a build with your v1 patch, but was unable to load any SSL site
> with it applied.

What exactly means 'unable to load' ?

	[:fabrice] Fabrice Desré
	Comment 13 • 14 years ago

(In reply to comment #12)
> (In reply to comment #11)
> > Honza, I did a build with your v1 patch, but was unable to load any SSL site
> > with it applied.
> 
> What exactly means 'unable to load' ?

The loading throbber runs forever, with no content being displayed.

	Honza Bambas (:mayhemer) Assignee
	Comment 14 • 14 years ago

Fabrice, would be good to check if this is not a dup of bug 592197, that already has a patch.

	[:fabrice] Fabrice Desré
	Comment 15 • 14 years ago

I tried several builds :

- patch in bug 592197 only : page loads, but doesn't display SSL Status

- patch in bug 592197 and Honza's : page does not load (we've seen in logs that we are actually doing two request, the first one being immediately canceled and the second one is stuck : the child process doesn't even get OnStartRequest)

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 16 • 14 years ago

What's the best way to proceed here? We can start snooping with gdb, but it would be nice to have some expectations for how it should be working.

Fabrice - I guess you could test the code paths in Firefox (or Fennec with browser.tabs.remote=false) and see how it is supposed to work. Then compare that to what you see when remote=true

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 14 years ago

Can we just deserialize the security info into a child-side-only impl?

	Honza Bambas (:mayhemer) Assignee
	Comment 18 • 14 years ago

(In reply to comment #17)
> Can we just deserialize the security info into a child-side-only impl?

Boris, sorry, I don't follow what you mean.  The security info is sent as a string-serialization to the child process through Send/RecvOnStartRequest.  The problem I was solving with my patch is that we cannot de/serialize certificates w/o having the nss initialized on the child process (bug 559711).

	Benjamin Smedberg
	Comment 19 • 14 years ago

I don't understand why the content process needs to know anything about it, other than perhaps storing a serialized certificate or a certificate handle and sending it back to the chrome process at some point. Although really, I'd prefer if the chrome process did all of the certificate tracking, but that probably requires mirrored load groups and such.

	Honza Bambas (:mayhemer) Assignee
	Comment 20 • 14 years ago

(In reply to comment #19)
> ...requires mirrored load groups and such.
Exactly.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 14 years ago

> The problem I was solving with my patch is that we cannot de/serialize
> certificates w/o having the nss initialized on the child process

Why not?

	Honza Bambas (:mayhemer) Assignee
	Comment 22 • 14 years ago

(In reply to comment #21)
> Why not?

It's using NSS code to parse DER data and create nss representation of the certificate, that nsNSSCertificate component only wraps.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 14 years ago

OK.  I assume that skipping all that and just having a non-NSS impl of nsIX509Cert is too much pain?

	Honza Bambas (:mayhemer) Assignee
	Comment 24 • 14 years ago

So, what you suggest is not to modify the current nsNSSCertificate code, but create a new class, registered under the same CID/contract, that is used on content (or in general - non-NSS) processes, and can be used just for carrying the serialization?

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 14 years ago

I'm saying it's my gut instinct to try that, yes.  I haven't thought about this enough to say whether I'd go so far as _suggesting_ it.  ;)

	Jason Duell
	Comment 26 • 14 years ago

Honza, how close are you with this?  Fennec needs this.

	Honza Bambas (:mayhemer) Assignee
	Comment 27 • 14 years ago

If it is that urgent I can have a patch today probably.

	Honza Bambas (:mayhemer) Assignee
	Comment 28 • 14 years ago

OK, work in progress, but finished probably tomorrow morning. Is that OK?

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 29 • 14 years ago

(In reply to comment #28)
> OK, work in progress, but finished probably tomorrow morning. Is that OK?

Should be fine - Thanks

	Honza Bambas (:mayhemer) Assignee
	Comment 30 • 14 years ago

Needs someone to test this please!  I don't have an environment to run my e10s patches at this time, and it doesn't look well to have one soon.  So, if anyone can take this patch and test it (actually run it for the first time) it would be of great help.  Thanks.

	[:fabrice] Fabrice Desré
	Comment 31 • 14 years ago

I'm doing a build right now.

	Stuart Parmenter
	Comment 32 • 14 years ago

Fabrice said the patch in this doesn't work -- Fabrice, do you have more info?

	[:fabrice] Fabrice Desré
	Comment 33 • 14 years ago

Here's the log I got loading https://demo.pairssl.com/ with the log set as :
NSPR_LOG_MODULES=timestamp,nsHttp:5,nsSocketTransport:5,nsSecureBrowserUI:5

SecurityUI info starts to appear at line 1053

The securityState appears to be 262146 (0x40002) which means STATE_IS_SECURE | STATE_SECURE_HIGH

I'll check again what the frontend is doing with this.

	[:fabrice] Fabrice Desré
	Comment 34 • 14 years ago

I did more testing :

- when tabs.remote == true, we get an assertion :
###!!! ASSERTION: Trying to initialize PSM/NSS in a non-chrome process!: 'Error', file /home/ifda7415/dev/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp, line 302
and subsequently JS errors : 
[Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIX509Cert.organization]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: chrome://browser/content/bindings/browser.js :: anonymous :: line 137"  data: no]

- when tabs.remote == false, it works except if we load an SSL page as the first page of a session (we get the SSL status for the subsequent loads).

	Honza Bambas (:mayhemer) Assignee
	Comment 35 • 14 years ago

Comment on attachment 470184 [details] [diff] [review]
v1

It was decided to use different classes for chrome and content process.  This patch doesn't do that.

	Honza Bambas (:mayhemer) Assignee
	Comment 36 • 14 years ago

Comment on attachment 475178 [details] [diff] [review]
v2

I have found out that on many places PSM calls new nsNSSCertificate.  Although those places seems as well dependent on NSS, so probably should NOT get invoked on child process thanks prevention of NSS instantiation, I'll wrap all new nsNSSCertificate to static nsNSSCertificate::Create method, that will return null on child process.

I already started to work on that patch.

	Honza Bambas (:mayhemer) Assignee
	Comment 37 • 14 years ago

Improved patch.

	Honza Bambas (:mayhemer) Assignee
	Comment 38 • 14 years ago

qref...

	Doug Turner (:dougt)
	Comment 39 • 14 years ago

honza, any update here?

	Honza Bambas (:mayhemer) Assignee
	Comment 40 • 14 years ago

Doug, the patch should fix the problem with serialization, but there seems to be some different problem in Fennec yet.

I don't have a working desktop build of Fennec to test, it seems the desktop build is broken...

Brian Crowder was ones helping me to find out what is wrong, but we've quit the work undone w/o a conclusion and we haven't meet again since than.

	Honza Bambas (:mayhemer) Assignee
	Comment 42 • 14 years ago

OK, I got my windows Fennec desktop build! credits: jdm, cjones.

Almost there, but few flaws:

1. I test with bugzilla.mozilla.org page, on its first load I don't get the
final STATE_STOP notification for the top level document, that would trigger
the OnSecurityChange notification from nsSecureBrowserUIImpl ; I get it on
reload.

Not sure where it gets lost, but will investigate further.  If anyone knows
something more, please make a note.

2. We correctly get here [1] with the patch but SecurityUI.getIdentityData
reads the certificate properties that are inaccessible on the content process
with the patch (like subjectName etc. as depending on NSS init).  

I need help with the second issue as I don't know the code enough.  The
notification passes this way:

1. /chrome/content/bindings/browser.js, onSecurityChange, the nsIWebProgress
imlementaion, called on content process
2. /chrome/content/bindings/browser.xml, remote-browser,
receiveMessage("WebProgress:SecurityChange"), _called on CHROME process ???_
3. /chrome/content/browser.js, onSecurityChange, the nsIWebProgress
imlementaion, called on the chrome process I assume

If the second step happens on the chrome process, we may send the serialization
and not the properties.  Then instantiate the whole security info, as the code
removed in bug 582840 did.

[1]
http://hg.mozilla.org/mobile-browser/annotate/450b16d94ac9/chrome/content/bindings/browser.js#l76

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 43 • 14 years ago

(In reply to comment #42)

> 2. /chrome/content/bindings/browser.xml, remote-browser,
> receiveMessage("WebProgress:SecurityChange"), _called on CHROME process ???_

Yes, browser.xml is in the chrome process.

	Jason Duell
	Comment 44 • 14 years ago

> 1. I test with bugzilla.mozilla.org page, on its first load I don't get the
final STATE_STOP notification for the top level document, that would trigger
the OnSecurityChange notification from nsSecureBrowserUIImpl ; I get it on
reload.

The last time I saw something like that, it turned out to be the channel not removing itself from the loadGroup.

	Honza Bambas (:mayhemer) Assignee
	Comment 45 • 14 years ago

I need to carry the certificate serialization between processes.  The code where it has to happen is, however, written in JS.  I don't know about any helper usable in JS that would make it simple to de/serialize an object to a string.


So, I plan on one of the following:

1. Create a new interface for nsNSSCertificate like nsINSSCertificateSerialization, that would access the cert serialization as a string, both classes (for chrome and content) would implement it.

2. Probably a better solution as we might want to carry also other objects in the future, create a component that could be used in JS to simply de/serialize objects, the same as functions from nsSerializationHelper.cpp do, but scriptable (a new bug for that).

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 46 • 14 years ago

(In reply to comment #45)
> I need to carry the certificate serialization between processes.  The code
> where it has to happen is, however, written in JS.  I don't know about any
> helper usable in JS that would make it simple to de/serialize an object to a
> string.

Honza - explain a little more about what you need and where the code (JS) is that needs to use it. We have a few message managers that can de/serialize object across processes.

We use them for helping in various JS XPCOM components. nsContentPrefService.js is one example:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/src/nsContentPrefService.js#50

	Honza Bambas (:mayhemer) Assignee
	Comment 47 • 14 years ago

On the content process I need to serialize the object through nsISerializable interface.  For nsNSSCertificate it is the only interface that is implemented on the content process as any other interfaces' properties need NSS initialized to return meaningful results, we don't have NSS on the content process, right? 

Then the result binary data, probably best converted to a base64 string, carry from the content process to the chrome process.  We already do this opposite way in C++ in PHttpChannel::OnStartRequest and we were doing this in PBrowser::NotifySecuryStateChange, that had been removed in bug 582840, and that I now need to re-create as part of this bug.

The exact place where I need to serialize the certificate is:
/chrome/content/bindings/browser.js, onSecurityChange (content process)

The exact place where I need to deserialize the certificate is:
/chrome/content/bindings/browser.xml, remote-browser,
receiveMessage("WebProgress:SecurityChange"), (chrome process, as you confirmed)

So, I just want to have a JS helper that makes it easier to convert an object implementing nsISerializable to a string and back.

	Honza Bambas (:mayhemer) Assignee
	Comment 48 • 14 years ago

I can't find anything in nsContentPrefService.js that I can use to do this.

	Honza Bambas (:mayhemer) Assignee
	Comment 49 • 14 years ago

Mozilla-central changes, wip patch.

	Honza Bambas (:mayhemer) Assignee
	Comment 50 • 14 years ago

Mobile changes.

Few questions:

1. How does Fennec display Extended Validation state (the green SSL UI) ?  I don't see it would be carried out in object returned from SecurityUI.getIdentityData().

2. What is the substitute for content.location (present in browser.js on the content process) on the chrome process in browser.xml?


IMPORTANT: When testing this patch, please remember it is not addressing the problem with missing STATE_STOP notification on the first page load, so you have to refresh the page to get the security UI.  That is probably a different bug.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 51 • 14 years ago

(In reply to comment #50)

> 1. How does Fennec display Extended Validation state (the green SSL UI) ?  I
> don't see it would be carried out in object returned from
> SecurityUI.getIdentityData().

Not exactly sure what you mean here, but Fennec also shows a green favicon for EV state. We use CSS on the "mode" attribute. See here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#1506

setMode will set the "mode" attribute to "verifiedIdentity" and CSS does the rest.

> 2. What is the substitute for content.location (present in browser.js on the
> content process) on the chrome process in browser.xml?

browser.currentURI.spec

	Honza Bambas (:mayhemer) Assignee
	Comment 52 • 14 years ago

Thanks for info.  Where exactly in the current code is the EV state handled?  EV status is not displayed with my patches, so it needs to be fixed as well.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 53 • 14 years ago

(In reply to comment #52)
> Thanks for info.  Where exactly in the current code is the EV state handled? 
> EV status is not displayed with my patches, so it needs to be fixed as well.

I don't know what you mean by "EV state handled" exactly so I am going to give you way to much information and hopefully something will answer your question.

If a website has an EV cert the favicon turns green and you tap/click the favicon you'll see the site identification panel. The EV strings for the panel are loaded here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#1615

The data used for the strings comes from <browser>.securityUI.SSLStatus.serverCert. That is originally set in content process here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/browser.js#84

And passed to the chrome process and held here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/browser.xml#56

One ugly aspect of this code (browser.xml#56) is that calling the getter actually starts the process of initializing the securityUI in the content process. The toolkit <browser> constructor seems to access the securityUI property and that makes it all work. Still kinda hacky IMO.

	Honza Bambas (:mayhemer) Assignee
	Comment 54 • 14 years ago

mark, thanks a lot, this is helpful.  One more question: where is this code [1] executed?  Chrome or content?

[1] http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#1491

	Honza Bambas (:mayhemer) Assignee
	Comment 55 • 14 years ago

This seems to be ready for review:
- full SSL status object serialization is carried from content to chrome process
- getIdentityData moved from content to chrome process, because on the chrome process there are no certificate properties to read, it caries only the serialization
- ev status is specially handled on the chrome process because NSS assistance is needed

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 56 • 14 years ago

Comment on attachment 478579 [details] [diff] [review]
v3.1 (mobile)


>+    let serialization = SecurityUI.getSSLStatusSerialization();

getSSLStatusSerialization -> getSSLStatus

"Serialization" is a bit too long

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml

>+                this._browser._securityUI = {
>+                  SSLStatus: SSLStatus ? {state: json.state, serverCert: data} : null,
>+                  state: json.state

                    SSLStatus: SSLStatus ? { serverCert: data } : null,

"state" does not need to be SSLStatus
I can make any changes on checkin, if needed

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 57 • 14 years ago

With these two patches and the patch in bug 599632, SSL "larry" works in Fennec

	Honza Bambas (:mayhemer) Assignee
	Comment 58 • 14 years ago

(In reply to comment #56)
> >+    let serialization = SecurityUI.getSSLStatusSerialization();
> 
> getSSLStatusSerialization -> getSSLStatus

I don't agree here.  SSLStatus means the object.  But what the method returns is the object's serialization.  I want to avoid any confusion here, so I'd rather leave the 'Serialization' here.  Or, would you be more OK with 'AsString' instead?

> "state" does not need to be SSLStatus

True, overlook.

Will update the patch.

	Honza Bambas (:mayhemer) Assignee
	Comment 59 • 14 years ago

Comment on attachment 478539 [details] [diff] [review]
v3 (m-c)

It turns out this is a full working version.

Kai: please review the security/ changes:
 - created a new class nsNSSCertificateChild that is instantiated under the same CID and ContractID on the _child_ process instead of nsNSSCertificate
 - some changes to nsNSSModule.cpp that helps to handle it

Boris: please review the netwerk/ changes:
 - just added an interface and component that does the same as serialization helper functions do but scripts can use it

To both: this patch is very simple although it looks large.

	Honza Bambas (:mayhemer) Assignee
	Comment 60 • 14 years ago

Serialization changed to AsString.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 61 • 14 years ago

Comment on attachment 482279 [details] [diff] [review]
v3.2 (mobile) [Check in comment 82]

AsString is fine by me

	Boris Zbarsky [:bzbarsky]
	Comment 62 • 14 years ago

Comment on attachment 478539 [details] [diff] [review]
v3 (m-c)

This new component should be in XPCOM, not necko, no?

	Kai Engert (:KaiE:)
	Comment 64 • 14 years ago

Comment on attachment 478539 [details] [diff] [review]
v3 (m-c)

(a)
> nsNSSCertificate*
> nsNSSCertificate::ConstructFromDER(char *certDER, int derLen)
> {
>-  nsNSSCertificate* newObject = new nsNSSCertificate();
>+#ifdef MOZ_IPC
>+  // On non-chrome process prevent instantiation
>+  if (GeckoProcessType_Default != XRE_GetProcessType())
>+    return nsnull;
>+#endif
>+
>+  nsNSSCertificate* newObject = nsNSSCertificate::Create();

While you're touching this, please add some kind of "newObject not null" check, before the following "->"

>   if (!newObject->InitFromDER(certDER, derLen)) {
>     delete newObject;
>     newObject = nsnull;
>   }


(b)
> nsNSSCertificate::nsNSSCertificate(CERTCertificate *cert) : 
>                                            mCert(nsnull),
>                                            mPermDelete(PR_FALSE),
>                                            mCertType(CERT_TYPE_NOT_YET_INITIALIZED),
>                                            mCachedEVStatus(ev_status_unknown)
> {
>+#ifdef MOZ_IPC
>+  if (GeckoProcessType_Default != XRE_GetProcessType())
>+    NS_ERROR("Trying to initialize nsNSSCertificate in a non-chrome process!");
>+#endif

NS_ERROR is a DebugBreak, only in debug builds, no action in optimized builds. Is this what you want?
If yes, let's avoid to evaluate the expression in optimized builds, maybe by wrapping in #ifdef DEBUG ?

(similar code more than once)


(c)
+  mCertSerialization = SECITEM_AllocItem(nsnull, nsnull, len);
+  if (!mCertSerialization)
+      return NS_ERROR_OUT_OF_MEMORY;
+  PORT_Memcpy(mCertSerialization->data, const_cast<char*>(str.get()), len);

I'm surprised you need a const_cast. I think str.get() should return (const char*) and second param to memcpy is a const char*.
What compiler error do you get if you remove the cast?


(d)
You have a comment that says:
+  // On a non-chrome process we don't have mCert because we lack
+  // nsNSSComponent.  nsNSSCertificateChild object is used only to carry the
+  // certificate serialization.

Let's use a more descriptive name than nsNSSCertificateChild.

I propose something like
  nsNSSCertFakeTransporter


(e)
+      // If the component needs PSM/NSS initialized only on the chrome process,
+      // pretend we successfully initiated it but in reality we bypass it.
+      // It's up to the programmer to check for process type in such components
+      // and take care not to call anything that needs NSS/PSM initiated.

scary!

Do you assume the current code will not call any of the non-implemented functions?

Did you try to replace
  return NS_ERROR_NOT_IMPLEMENTED;
with
  PR_Assert(0);
to see if you get an abort?


(f)
The changes to the factory constructors looked scary, too, on first sight, but I couldn't spot any obvious mistakes.


(g)
What do you think about bz's proposal to move the netwerk/ code to xpcom/ ?

	Honza Bambas (:mayhemer) Assignee
	Comment 66 • 14 years ago

(In reply to comment #62)
> Comment on attachment 478539 [details] [diff] [review]
> v3 (m-c)
> 
> This new component should be in XPCOM, not necko, no?

It's using NS_SerializeToString and NS_DeserializeObject that are both declared/defined in netwerk.  So, I'd argue to leave it in netwerk.

	Boris Zbarsky [:bzbarsky]
	Comment 67 • 14 years ago

> It's using NS_SerializeToString and NS_DeserializeObject that are both
> declared/defined in netwerk.

Hmph.  Those should live in xpcom too..

Maybe file a followup on moving that all there?

	Honza Bambas (:mayhemer) Assignee
	Comment 68 • 14 years ago

(In reply to comment #67)
> Maybe file a followup
Bug 605995.

	Honza Bambas (:mayhemer) Assignee
	Comment 69 • 14 years ago

(In reply to comment #64)
> (e)
> scary!
> 
> Do you assume the current code will not call any of the non-implemented
> functions?
> 
> Did you try to replace
>   return NS_ERROR_NOT_IMPLEMENTED;
> with
>   PR_Assert(0);
> to see if you get an abort?
> 

I went through mxr checking all properties that are unimplemented by this new class.  No-one is accessing them on the content process.

	Honza Bambas (:mayhemer) Assignee
	Comment 70 • 14 years ago

(In reply to comment #68)
> (In reply to comment #67)
> > Maybe file a followup
> Bug 605995.

Boris, I'll move the component in that bug.  Otherwise the code is OK?  I'll soon update the patch with Kai's comments.

	Honza Bambas (:mayhemer) Assignee
	Comment 71 • 14 years ago

kaie: a,b,c,d addressed. e explained in comment 69
boris: please review only the network changes

	Boris Zbarsky [:bzbarsky]
	Comment 73 • 14 years ago

> Otherwise the code is OK? 

I need to look again; I forget whether I read the rest of it the first time...

	Kai Engert (:KaiE:)
	Comment 75 • 14 years ago

Comment on attachment 485144 [details] [diff] [review]
v3.1 (mozilla-central) [Check in comment 80]

Thanks, r=kaie

	Kai Engert (:KaiE:)
	Comment 76 • 14 years ago

(In reply to comment #75)
> 
> Thanks, r=kaie

(r= for the changes in security, I didn't look at netwerk)

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 77 • 14 years ago

Testers note: please verify theme change : Bug 575403 
when regressing this bug in locked, unlocked, invalid cert, etc.

	Boris Zbarsky [:bzbarsky]
	Comment 78 • 14 years ago

Comment on attachment 485144 [details] [diff] [review]
v3.1 (mozilla-central) [Check in comment 80]

r=me, though this new service seems to be unused and untested...  I assume there are plans to change both?

	Honza Bambas (:mayhemer) Assignee
	Comment 79 • 14 years ago

(In reply to comment #78)
> Comment on attachment 485144 [details] [diff] [review]
> v3.1 (mozilla-central)
> 
> r=me, though this new service seems to be unused and untested...  I assume
> there are plans to change both?

It is used in the mobile patch.

	Honza Bambas (:mayhemer) Assignee
	Comment 80 • 14 years ago

Comment on attachment 485144 [details] [diff] [review]
v3.1 (mozilla-central) [Check in comment 80]

http://hg.mozilla.org/mozilla-central/rev/45376975eca8

	Honza Bambas (:mayhemer) Assignee
	Comment 81 • 14 years ago

And a bustage fix for gcc builds:
http://hg.mozilla.org/mozilla-central/rev/116ab7c785e1

	Honza Bambas (:mayhemer) Assignee
	Comment 82 • 14 years ago

Comment on attachment 482279 [details] [diff] [review]
v3.2 (mobile) [Check in comment 82]

http://hg.mozilla.org/mobile-browser/rev/c2cfa42b9157

	Honza Bambas (:mayhemer) Assignee
	Comment 83 • 14 years ago

Patches landed on m-c and mobile.  Marking fixed.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 84 • 14 years ago

Green cert can be seen by going to something like paypal.com
Blue cert can be seen by going to something like gmail.com

Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre) Gecko/20101029 Firefox/4.0b8pre Fennec/4.0b2pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101029 Firefox/4.0b8pre Fennec/4.0b2pre