Last Comment Bug 722979 - nsStrictTransportSecurityService uses the global Private Browsing service to decide where to set permissions
: nsStrictTransportSecurityService uses the global Private Browsing service to ...
Status: RESOLVED FIXED
: addon-compat, APIchange, dev-doc-needed
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla19
Assigned To: Josh Matthews [:jdm]
:
Mentors:
Depends on: 722845 725210 812794 819106
Blocks: PBnGen 769288 mobilepb 806731 806732 806733
  Show dependency treegraph
 
Reported: 2012-01-31 21:53 PST by Josh Matthews [:jdm]
Modified: 2012-12-06 14:28 PST (History)
15 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add privacy status argument to relevant nsIStrictTransportSecurityService methods. (51.08 KB, patch)
2012-06-30 07:39 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Add privacy status argument to relevant nsIStrictTransportSecurityService methods. (62.51 KB, patch)
2012-09-21 13:48 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Add privacy status argument to relevant nsIStrictTransportSecurityService methods. (69.38 KB, patch)
2012-10-29 13:14 PDT, Josh Matthews [:jdm]
brian: review-
bzbarsky: superreview+
Details | Diff | Splinter Review
Add privacy status argument to relevant nsIStrictTransportSecurityService methods. (68.27 KB, patch)
2012-11-07 17:47 PST, Josh Matthews [:jdm]
brian: review+
mcmanus: review+
Details | Diff | Splinter Review
Add privacy status argument to relevant nsIStrictTransportSecurityService methods. (69.25 KB, patch)
2012-11-14 11:48 PST, Josh Matthews [:jdm]
cbiesinger: superreview+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2012-01-31 21:53:47 PST
The global PB service is going away. The existing interface just takes URIs; we may need to add specific interfaces to access private vs. public permissions.
Comment 1 Josh Matthews [:jdm] 2012-04-11 12:38:39 PDT
Given that the only caller is nsHttpChannel, I think we can potentially modify the interface for ProcessStsHeader to accept an nsIPrivateBrowsingConsumer argument and use that to determine the privacy state. Honza, does that sound reasonable?
Comment 2 Honza Bambas (:mayhemer) 2012-04-12 07:43:33 PDT
Rather a new method ProcessStsHeaderWithPrivateState (or so) ?  The current one will assume PB=off.
Comment 3 Josh Matthews [:jdm] 2012-04-12 07:52:09 PDT
Any particular reason we want two different methods? The single method could accept a null pointer if necessary.
Comment 4 Honza Bambas (:mayhemer) 2012-04-12 07:55:20 PDT
(In reply to Josh Matthews [:jdm] from comment #3)
> Any particular reason we want two different methods? The single method could
> accept a null pointer if necessary.

Just preserve compatibility for extensions.  That's all.  But making the arg just optional may be the solution here.  Yep, let's do it.
Comment 5 :Ehsan Akhgari 2012-04-12 11:19:23 PDT
(In reply to Honza Bambas (:mayhemer) from comment #4)
> (In reply to Josh Matthews [:jdm] from comment #3)
> > Any particular reason we want two different methods? The single method could
> > accept a null pointer if necessary.
> 
> Just preserve compatibility for extensions.  That's all.  But making the arg
> just optional may be the solution here.  Yep, let's do it.

Note that it would only help with js add-ons, binary add-ons would still need to be recompiled.
Comment 6 Honza Bambas (:mayhemer) 2012-04-12 11:43:42 PDT
Options:
1. preserve binary compatibility: new interface
2. preserve JS compatibility: new method or default-valued arg
3. no preservation at all: just modify the method's signature

I don't know what the policy is now.  I see patches doing (2) quit often these days.
Comment 7 :Ehsan Akhgari 2012-04-12 12:38:51 PDT
Honestly in this case I'd really prefer if we break both binary and js compatibility, so that extensions using this interface start to be PB aware.  I don't think there are too many extensions relying on this, which is heavily biasing my judgement towards breaking backwards compat.
Comment 8 Josh Matthews [:jdm] 2012-05-25 08:32:20 PDT
Hmm, I wonder if more users of the service have appeared in the paste 1.5 months? It looks like we'll need to add arguments to a bunch of interface methods:
* RemoveStsState
* IsStsURI
* IsStsHost
* ProcessStsHeader

We should probably make the extra argument nsILoadContext so we can easily update consumers like http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_websocket.html?force=1#1226.
Comment 9 Josh Matthews [:jdm] 2012-05-25 08:36:53 PDT
Brian, is there some way to get to a logical docshell or channel in the cert verification code at http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#307?
Comment 10 Josh Matthews [:jdm] 2012-05-25 08:39:32 PDT
According to the AMO mxr, Sid's Force-TLS addon is the only one that uses this interface. All in-tree users appear here: http://mxr.mozilla.org/mozilla-central/search?string=nsistricttransport
Comment 11 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-05-25 09:56:11 PDT
(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #9)
> Brian, is there some way to get to a logical docshell or channel in the cert
> verification code at
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> SSLServerCertVerification.cpp#307?

There should not be any uses of the docshell in the SSL or cert verification code. I am working on removing the last usage of docshell in that code now.

Instead, you need to pass the information for strict transport security when you create the transport layer. Camilo has a very similar issue and he already is modifying the code to pass additional information at the time the socket transport is created. You and him should be able to use basically the exact same technique.

We need the private browsing information earlier anyway, so that we can fix bugs about writing SSL-related data to disk in private browsing mode.
Comment 12 :Ehsan Akhgari 2012-05-25 10:22:47 PDT
(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #10)
> According to the AMO mxr, Sid's Force-TLS addon is the only one that uses
> this interface. All in-tree users appear here:
> http://mxr.mozilla.org/mozilla-central/search?string=nsistricttransport

I'm sure Sid will happily change his add-on if we ask him nicely.  :-)
Comment 13 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-05-25 12:02:03 PDT
(In reply to Honza Bambas (:mayhemer) from comment #6)
> Options:
> 1. preserve binary compatibility: new interface
> 2. preserve JS compatibility: new method or default-valued arg
> 3. no preservation at all: just modify the method's signature
> 
> I don't know what the policy is now.  I see patches doing (2) quit often
> these days.

In this case, I think it is OK to go with #3 and ignore addon compatibility.

I am not sure it is OK to do #1 or #2. We want all addons to respect the private browsing mode settings for each tab, right? If so, then any time that cannot happen automatically, we must change the signatures of the methods/interfaces used by addons so that the addons will break until they implement the correct private browsing mode behavior.
Comment 14 :Ehsan Akhgari 2012-05-25 14:07:31 PDT
I'd rather us break add-ons here.
Comment 15 Sid Stamm [:geekboy or :sstamm] 2012-05-25 14:13:06 PDT
Yeah, break compatibility with the add-on that uses it.  I'll fix the add-on.
Comment 16 Josh Matthews [:jdm] 2012-06-26 13:31:27 PDT
(In reply to Brian Smith (:bsmith) from comment #11)
> (In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment
> #9)
> > Brian, is there some way to get to a logical docshell or channel in the cert
> > verification code at
> > http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> > SSLServerCertVerification.cpp#307?
> 
> There should not be any uses of the docshell in the SSL or cert verification
> code. I am working on removing the last usage of docshell in that code now.
> 
> Instead, you need to pass the information for strict transport security when
> you create the transport layer. Camilo has a very similar issue and he
> already is modifying the code to pass additional information at the time the
> socket transport is created. You and him should be able to use basically the
> exact same technique.
> 
> We need the private browsing information earlier anyway, so that we can fix
> bugs about writing SSL-related data to disk in private browsing mode.

Brian, I haven't been in contact with Camilo about his changes yet, but I'm worried by this response. Isn't the socket transport creation a one-time event? We need to be able to determine the privacy status of the window that initiated the connection that had a cert error, an action which is necessarily dynamic.
Comment 17 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-26 13:58:57 PDT
(In reply to Josh Matthews [:jdm] from comment #16)
> Brian, I haven't been in contact with Camilo about his changes yet, but I'm
> worried by this response. Isn't the socket transport creation a one-time
> event? We need to be able to determine the privacy status of the window that
> initiated the connection that had a cert error, an action which is
> necessarily dynamic.

I think we just have a little miscommunication.

We need to know whether the connection should be a "private" one at the time where we create and/or choose the connection to send the request on. If we need a "private" connection and there are no suitable private connections, we must create a new one; when we do so, we should pass all the information that private browsing and strict transport security needs at that time. If there's an existing "private" connection then we can just put the transaction on that connection without any problems.

In particular, I am expecting that we will pass the private browsing information (just "bool isPrivate"?) in new parameters of nsSSLIOLayerNewSocket and nsSSLIOLayerAddToSocket.

In fact, perhaps we should just change the nsISocketProvider API to add the "isPrivate" flag as a parameter newSocket and addToSocket, so that all providers and all users of the providers are required to explicitly understand private browsing mode.
Comment 18 :Ehsan Akhgari 2012-06-28 14:39:33 PDT
Josh, please let us know if you want Chris to take this!
Comment 19 Josh Matthews [:jdm] 2012-06-28 21:41:09 PDT
I'm pretty sure I can tackle this in the next couple days.
Comment 20 Josh Matthews [:jdm] 2012-06-30 07:39:57 PDT
Created attachment 638110 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.
Comment 21 Mozilla RelEng Bot 2012-06-30 11:15:23 PDT
Try run for 5e54241800b4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5e54241800b4
Results (out of 132 total builds):
    success: 77
    warnings: 54
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-5e54241800b4
Comment 22 Mozilla RelEng Bot 2012-06-30 15:15:21 PDT
Try run for 5a3f87205068 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5a3f87205068
Results (out of 133 total builds):
    success: 99
    warnings: 33
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-5a3f87205068
Comment 23 Josh Matthews [:jdm] 2012-09-21 13:48:32 PDT
Created attachment 663548 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.
Comment 24 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-21 14:21:43 PDT
See the change to nsISocketProvider.idl. When you create sockets, you need to specify whether or not the socket should be considered to be in "private browsing." I guess mailnews doesn't have any concept of "private browsing" so you can just pass "false," if you are using these APIs directly.
Comment 25 neil@parkwaycc.co.uk 2012-09-21 16:24:43 PDT
Comment on attachment 663548 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

>-        connector.speculativeConnect(searchURI, null, null);
>+        let callbacks = window.getInterface(Ci.nsIWebNavigation)
>+                          .QueryInterface(Ci.nsIDocShell)
>+                          .currentDocumentChannel.notificationCallbacks;
>+        connector.speculativeConnect(searchURI, callbacks, null);
Is the window's own nsIInterfaceRequestor implementation incapable of handling the request? (Nit: you forgot to QueryInterface to nsIInterfaceRequestor but you were probably saved by some other browser code doing that for you.)
Comment 26 Josh Matthews [:jdm] 2012-09-21 19:50:59 PDT
I'm not sure what you're asking. People like bz and gavin have always told me that this is the series of steps requred to obtain a docshell.
Comment 27 Josh Matthews [:jdm] 2012-09-22 15:04:01 PDT
https://tbpl.mozilla.org/?tree=Try&rev=686550d94acf
Comment 28 neil@parkwaycc.co.uk 2012-09-30 09:39:45 PDT
(In reply to Josh Matthews from comment #26)
> I'm not sure what you're asking. People like bz and gavin have always told
> me that this is the series of steps requred to obtain a docshell.

Sorry, I misread the code, I overlooked that you wanted the channel's callbacks.
(The window.QueryInterface(Ci.nsIInterfaceRequestor) is still technically missing though.)
Comment 29 Josh Matthews [:jdm] 2012-10-29 13:14:08 PDT
https://tbpl.mozilla.org/?tree=Try&rev=14705b6d63c3
Comment 30 Josh Matthews [:jdm] 2012-10-29 13:14:20 PDT
Created attachment 676282 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.
Comment 31 Josh Matthews [:jdm] 2012-10-29 13:15:27 PDT
Comment on attachment 676282 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

Boris, can you approve the interface changes?
Comment 32 Josh Matthews [:jdm] 2012-10-29 13:17:24 PDT
Note to self: rev the UUIDs.
Comment 33 Boris Zbarsky [:bz] 2012-10-29 18:25:13 PDT
Comment on attachment 676282 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

Once bug 804495 lands you should be able to GetInterface an nsILoadContext directly from a window.

Interface changes look fine.
Comment 34 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-11-07 13:57:12 PST
Comment on attachment 676282 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

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

::: browser/components/search/content/search.xml
@@ +500,5 @@
>          var searchURI = engine.getSubmission("dummy").uri;
> +        let callbacks = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                              .getInterface(Components.interfaces.nsIWebNavigation)
> +                              .QueryInterface(Components.interfaces.nsIDocShell)
> +                              .currentDocumentChannel.notificationCallbacks;

This part I am not sure about, because I don't know if this is the right notificationCallbacks to be retrieving (or even what this will retrieve). Please ask somebody who knows such things to review this chunk.

::: docshell/base/nsDocShell.cpp
@@ +4171,5 @@
>                  NS_ENSURE_SUCCESS(rv, rv);
>  
>                  bool isStsHost = false;
> +                uint32_t flags =
> +                    mInPrivateBrowsing ? nsIStrictTransportSecurityService::IS_PRIVATE : 0;

uint32_t flags = tsi->GetTransportFlags();

(See comments below; this assumes we add a [notxpcom] GetTransportFlags() method to nsITransportSecurityInfo.)

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +976,5 @@
>              
>              if (mConnectionFlags & nsISocketTransport::ANONYMOUS_CONNECT)
>                  proxyFlags |= nsISocketProvider::ANONYMOUS_CONNECT;
>  
> +            bool isPrivate = !!(mConnectionFlags & nsISocketTransport::IS_PRIVATE);

proxyFlags |= (mConnectionlags & (nsISocketTransport::IS_PRIVATE);

@@ +984,5 @@
>                  // if this is the first type, we'll want the 
>                  // service to allocate a new socket
>                  rv = provider->NewSocket(mNetAddr.raw.family,
>                                           host, port, proxyHost, proxyPort,
> +                                         proxyFlags, isPrivate, &fd,

isPrivate should just be a flag incorporated into proxyFlags.

We must have had some misunderstanding about what boolean argument I wanted you to convert to a flag. I wasn't asking for you to change nsIStrictTransportSecurityService's boolean isPrivate argument to a flag; I was asking for *this* isPrivate argument to be incorporated into the flags argument (proxyFlags in this case) so that we don't have to change the nsISocket* interfaces at all (besides adding the new flag constant).

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1114,5 @@
>      // If this wasn't an STS host, errors are allowed, but no more STS processing
>      // will happen during the session.
>      bool wasAlreadySTSHost;
> +    uint32_t flags =
> +      NS_UsePrivateBrowsing(this) ? nsIStrictTransportSecurityService::IS_PRIVATE : 0;

It is good to pass flags to nsIStrictTransportSecurityService. But, why not just have nsIStrictTransportSecurityService use nsISocketTransport's flags, instead of having its own? Otherwise, we have to translate between the two, right?

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +227,5 @@
>    if (aIncludeSubdomains != nullptr) {
>      *aIncludeSubdomains = false;
>    }
>  
> +  bool isPrivate = aFlags & nsIStrictTransportSecurityService::IS_PRIVATE;

It seems like you should just have ProcessStsHeaderMutating take the flags itself, instead of converting to boolean here. Notice in ProcessStsHeaderMutating -> SetStsStat you end up converting the boolean back into flags anyway.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +292,5 @@
>    bool strictTransportSecurityEnabled = false;
>    nsCOMPtr<nsIStrictTransportSecurityService> stss
>      = do_GetService(NS_STSSERVICE_CONTRACTID, &nsrv);
>    if (NS_SUCCEEDED(nsrv)) {
> +    uint32_t flags = mInfoObject->IsPrivate() ? nsIStrictTransportSecurityService::IS_PRIVATE : 0;

We should just store the transport flags in mInfoObject, and then have a GetTransportFlags() that we can then pass the result of to IsStsHost.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1312,5 @@
>                        PRFileDesc **fd,
>                        nsISupports** info,
>                        bool forSTARTTLS,
> +                      bool anonymousLoad,
> +                      bool isPrivate)

Again, instead of converting the transport flags into booleans, we should just pass them as a single flags parameter, and then extract out the anonymousLoad and/or isPrivate flags when necessary; e.g. bool IsPrivate() { return mLoadFags & nsISocketTransport::IS_PRIVATE; }

@@ +2436,5 @@
>    PRFileDesc* layer = nullptr;
>    nsresult rv;
>    PRStatus stat;
>  
>    nsNSSSocketInfo* infoObject = new nsNSSSocketInfo();

new nsNSSSocketInfo(flags);
Comment 35 Josh Matthews [:jdm] 2012-11-07 17:47:03 PST
Created attachment 679486 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

I believe this addresses all of your points.
Comment 36 Josh Matthews [:jdm] 2012-11-07 17:48:10 PST
Comment on attachment 679486 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

Gavin, can you sign off on the browser/ change?
Comment 37 Josh Matthews [:jdm] 2012-11-07 17:50:16 PST
And yes, I need to change one stale reference to nsIStrictTransportService.IS_PRIVATE in test_websocket.html. I've done that locally.
Comment 38 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-11-08 08:47:09 PST
Comment on attachment 679486 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

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

LGTM with just one notable concern. Please ask biesi about moving the nsISocketProvider flags to nsISocketTransport.

::: netwerk/base/public/nsISocketTransport.idl
@@ +151,5 @@
>      const unsigned long DISABLE_IPV6 = (1 << 2);
>  
>      /**
> +     * If set, indicates that the connection was initiated from a source
> +     * defined as being private.

We should expand on this to help future Necko peers understand what "private" means. Something like s/private./private in the sense of Private Browsing. Generally, this there should be no state shared between connections that are private and connections that are not private; it is OK for multiple private connections to share state with each other, and it is OK for multiple non-private connections to share state with each other."

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +977,5 @@
>              if (mConnectionFlags & nsISocketTransport::ANONYMOUS_CONNECT)
>                  proxyFlags |= nsISocketProvider::ANONYMOUS_CONNECT;
>  
> +            if (mConnectionFlags & nsISocketTransport::IS_PRIVATE)
> +                proxyFlags |= nsISocketTransport::IS_PRIVATE;

Oops. I overlooked this in my previous comment. nsISocketTransport flags are slightly different than nsISocketProvider flags.

In order to avoid confusion and duplicates, I think we should first move nsISocketProvider.PROXY_RESOLVES_HOST to nsISocketTransport, with a different value (since 1 << 0 is already used for BYPASS_CACHE), and then remove nsISocketProvider.ANONYMOUS_CONNECT (using nsISocketTransport.ANONYMOUS_CONNECT, which happens to have the same value). Then, for flags that only apply to transports or providers, we can simply document that fact in the IDL, while flags that apply to both (ANONYMOUS_CONNECT and IS_PRIVATE) are guaranteed to not collide when used interchangeably.

::: security/manager/ssl/src/TransportSecurityInfo.h
@@ +89,5 @@
>    mutable ::mozilla::Mutex mMutex;
>  
>  protected:
>    nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
> +  uint32_t mTransportFlags;

const uint32_t mTransportFlags;

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +76,5 @@
>      mFirstServerHelloReceived(false),
>      mNPNCompleted(false),
>      mHandshakeCompleted(false),
>      mJoined(false),
>      mSentClientCert(false)

mTransportFlags(transportFlags)
Comment 39 Josh Matthews [:jdm] 2012-11-08 12:50:50 PST
Christian, what do you think of bsmith's suggestion of moving flags around in comment 38?
Comment 40 Josh Matthews [:jdm] 2012-11-08 13:04:59 PST
>::: security/manager/ssl/src/TransportSecurityInfo.h
>@@ +89,5 @@
>>    mutable ::mozilla::Mutex mMutex;
>>  
>>  protected:
>>    nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
>> +  uint32_t mTransportFlags;
>
>const uint32_t mTransportFlags;
>
>::: security/manager/ssl/src/nsNSSIOLayer.cpp
>@@ +76,5 @@
>>      mFirstServerHelloReceived(false),
>>      mNPNCompleted(false),
>>      mHandshakeCompleted(false),
>>      mJoined(false),
>>      mSentClientCert(false)
>
>mTransportFlags(transportFlags)

I can't make these changes, since inherited members can't be used in initializer lists, and I can't add the flags to the constructor for TransportSecurityInfo since there's a generic factory function for the class.
Comment 41 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-11-08 13:28:47 PST
Comment on attachment 679486 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +370,5 @@
>          NS_ENSURE_TRUE(stss, NS_ERROR_OUT_OF_MEMORY);
>  
>          bool isStsHost = false;
> +        uint32_t flags =
> +          NS_UsePrivateBrowsing(this) ? nsISocketTransport::IS_PRIVATE : 0;

Everywhere you are using NS_usePrivateBrowsing in nsHttpChanel, you can just use mPrivateBrowsing, right? It seems better to use mPrivateBrowsing since NS_UsePrivateBrowsing() is non-trivial.
Comment 42 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-11-09 12:11:49 PST
(In reply to Josh Matthews [:jdm] from comment #40)
> I can't make these changes, since inherited members can't be used in
> initializer lists, and I can't add the flags to the constructor for
> TransportSecurityInfo since there's a generic factory function for the class.

OK, I forgot about the splitting of TransportSecurityInfo and nsNSSSocketinfo and what it means. We cannot put the flags in TransportSecurityInfo because we need to be able to serialize/deserialize TransportSecurityInfo from the cache, but we can't serialize/deserialize the flags. Either we can put the flags accessor in nsISSLSocketControl, or we can just use your original idea of checking if the current tab/window is a private browsing tab/window. Probably your original idea is better.
Comment 43 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-10 14:16:15 PST
Comment on attachment 679486 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

I'm not really clear on what implications passing the chrome window's nsILoadContext to speculativeConnect() may have, beyond the obvious one being added in this patch (its use in nsHttpHandler::SpeculativeConnect). I traced its other use in this code path to NullHttpTransaction::GetSecurityCallbacks, but figuring out what callers of that might use it for isn't trivial. Someone who knows that that's "safe" should sign off on this - I don't know who that would be.
Comment 44 Josh Matthews [:jdm] 2012-11-10 19:57:30 PST
Comment on attachment 679486 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

Patrick, can you comment on the change in the caller of speculativeConnect in search.xml?
Comment 45 Josh Matthews [:jdm] 2012-11-12 04:50:48 PST
(In reply to Brian Smith (:bsmith) from comment #42)
> (In reply to Josh Matthews [:jdm] from comment #40)
> > I can't make these changes, since inherited members can't be used in
> > initializer lists, and I can't add the flags to the constructor for
> > TransportSecurityInfo since there's a generic factory function for the class.
> 
> OK, I forgot about the splitting of TransportSecurityInfo and
> nsNSSSocketinfo and what it means. We cannot put the flags in
> TransportSecurityInfo because we need to be able to serialize/deserialize
> TransportSecurityInfo from the cache, but we can't serialize/deserialize the
> flags. Either we can put the flags accessor in nsISSLSocketControl, or we
> can just use your original idea of checking if the current tab/window is a
> private browsing tab/window. Probably your original idea is better.

I went with nsISSLSocketControl, otherwise I needed to add another static_cast to nsNSSSocketInfo in SSLServerCertVerification, since we need the flags there.
Comment 46 Josh Matthews [:jdm] 2012-11-13 06:13:27 PST
Formalizing the request from comment 38 and comment 39. The flags issue is the only thing blocking this patch from landing now.
Comment 47 Josh Matthews [:jdm] 2012-11-14 11:48:50 PST
Created attachment 681597 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.
Comment 48 Josh Matthews [:jdm] 2012-11-14 11:52:13 PST
Comment on attachment 681597 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

Christian, if you could approve the API changes, I would appreciate it. In particular, I ended up following the existing convention and having an IS_PRIVATE flag in both nsISocketTransport and nsISocketProvider. I think that is the easier route to take at this point, rather than trying to set up a flag sharing scheme between them.
Comment 49 Christian :Biesinger (don't email me, ping me on IRC) 2012-11-15 10:43:10 PST
Comment on attachment 681597 [details] [diff] [review]
Add privacy status argument to relevant nsIStrictTransportSecurityService methods.

+++ b/netwerk/base/public/nsIStrictTransportSecurityService.idl

this file seriously needs to document flags, since it doesn't have any flags in the IDL itself. where do the valid flags come from?

I don't think sharing flags between the two interfaces is a good idea, I prefer the approach in this patch. Although I don't like the name IS_PRIVATE... maybe NO_PERSISTENT_STORAGE or something along those lines?

+++ b/netwerk/socket/nsISSLSocketControl.idl

need new IID
Comment 52 Josh Matthews [:jdm] 2012-11-16 08:59:55 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/d86aa72c7dbf should do it this time.
Comment 53 Ryan VanderMeulen [:RyanVM] 2012-11-16 19:08:39 PST
https://hg.mozilla.org/mozilla-central/rev/d86aa72c7dbf

Note You need to log in before you can comment on or make changes to this bug.