1282124 - Remove SEC_FORCE_PRIVATE_BROWSING Flag

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[James Andreou]

With the new OriginAttribute mPrivateBrowsingId we can remove nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING and therefore also remove LoadInfo::GetUsePrivateBrowsing

Comment 1

[James Andreou]

Attachment: Bug1281224.patch

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8765004 [details] [diff] [review]
Bug1281224.patch

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

More than a review, is a list of questions :)

::: image/test/unit/test_private_channel.js
@@ -57,5 @@
>    var uri = NetUtil.newURI(gImgPath);
>    var securityFlags = Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;
> -  if (isPrivate) {
> -    securityFlags |= Ci.nsILoadInfo.SEC_FORCE_PRIVATE_BROWSING;
> -  }

I don't understand how this works. Here we don't set the flag, ok, and we end up using gPrivateLoader.
Previously we called respectPrivacyNotifications() and imgLoader::mRespectPrivacy is set to true.
But the channel doesn't receive this flag anymore with this patch. So, why should this work?

::: netwerk/base/LoadInfo.cpp
@@ +173,5 @@
> +      aLoadingContext->OwnerDoc()->GetLoadContext();
> +    if (loadContext) {
> +      DocShellOriginAttributes attrs;
> +      if (loadContext->GetOriginAttributes(attrs)) {
> +        mOriginAttributes.mPrivateBrowsingId = attrs.mPrivateBrowsingId;

Why should all of this needed?
And why just privateBrowsingId? Should we use the all loadContext originAttribute?

And... why this loadContext is != than mLoadingPrincipal originAttribute?

::: netwerk/base/nsNetUtil.cpp
@@ +2430,5 @@
>                 "loadInfo are not the same!");
>  
> +    MOZ_ASSERT(originAttrsLoadInfo.mPrivateBrowsingId ==
> +               originAttrsLoadContext.mPrivateBrowsingId,
> +               "The value of mPrivateBrowsingId in the loadContext and in the loadInfo "

Can you file a follow up to make this comparison simpler?
In particular I would like to see why/if we can just do:

    OriginAttributes originAttrsLoadInfo = loadInfo->GetOriginAttributes();
    DocShellOriginAttributes originAttrsLoadContext;
    loadContext->GetOriginAttributes(originAttrsLoadContext);

    MOZ_ASSERT(ChromeUtils::IsOriginAttributesEqual(originAtrsLoadInfo, originAttrsLoadContext));

or maybe... IsOriginAttributesEqualIgnoringAddonId.

Comment 3

[James Andreou]

image/test/unit/test_private_channel.js
@@ -57,5 @@

The test sets the NotificationCallback's usePrivateBrowsing which will result in NS_UsePrivateBrowsing(channel) being true.

::: netwerk/base/LoadInfo.cpp
@@ +173,5 @@
I am convinced we do not need it!

::: netwerk/base/nsNetUtil.cpp
@@ +2430,5 @@
I will file a follow up bug to try and make the comparison solely an OA equality if possible.

Comment 4

[James Andreou]

Attachment: Bug1281183.patch

Comment 5

[James Andreou]

Attachment: Bug1282124.patch

Corrected Bug #

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8766386 [details] [diff] [review]
Bug1282124.patch

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

Looks good to me! But you need a necko/devtools/img peer(s).

Comment 7

[James Andreou]

Comment on attachment 8766386 [details] [diff] [review]
Bug1282124.patch

># HG changeset patch
># User James Andreou <[e-mail]>
>
>Bug 1282124 - Remove Notfication PB Flags r=baku
>
>
>diff --git a/devtools/shared/DevToolsUtils.js b/devtools/shared/DevToolsUtils.js
>index 3be8c16..f15271e 100644
>--- a/devtools/shared/DevToolsUtils.js
>+++ b/devtools/shared/DevToolsUtils.js
>@@ -514,27 +514,6 @@ function mainThreadFetch(aURL, aOptions = { loadFromCache: true,
>  */
> function newChannelForURL(url, { policy, window, principal }) {
>   var securityFlags = Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;
>-  if (window) {
>-    // Respect private browsing.
>-    var req = window.QueryInterface(Ci.nsIInterfaceRequestor)
>-                    .getInterface(Ci.nsIWebNavigation)
>-                    .QueryInterface(Ci.nsIDocumentLoader)
>-                    .loadGroup;
>-    if (req) {
>-      var nc = req.notificationCallbacks;
>-      if (nc) {
>-        try {
>-          var lc = nc.getInterface(Ci.nsILoadContext);
>-          if (lc) {
>-            if (lc.usePrivateBrowsing) {
>-              securityFlags |= Ci.nsILoadInfo.SEC_FORCE_PRIVATE_BROWSING;
>-            }
>-          }
>-        } catch (ex) {}
>-      }
>-    }
>-  }
>-
>   let channelOptions = {
>     contentPolicyType: policy,
>     securityFlags: securityFlags,
>@@ -547,7 +526,35 @@ function newChannelForURL(url, { policy, window, principal }) {
>     }
>     channelOptions.loadingPrincipal = principal;
>   } else {
>-    channelOptions.loadUsingSystemPrincipal = true;
>+    // We need to pass private browsing to the channel's load info, if a private
>+    // load context exists on the window.
>+    let attrs = { privateBrowsingId : 0 };
>+    if (window) {
>+      var req = window.QueryInterface(Ci.nsIInterfaceRequestor)
>+                      .getInterface(Ci.nsIWebNavigation)
>+                      .QueryInterface(Ci.nsIDocumentLoader)
>+                      .loadGroup;
>+      if (req) {
>+        var nc = req.notificationCallbacks;
>+        if (nc) {
>+          try {
>+            var lc = nc.getInterface(Ci.nsILoadContext);
>+            if (lc) {
>+              attrs = lc.originAttributes;
>+            }
>+          } catch (ex) {}
>+        }
>+      }
>+    }
>+
>+    if (attrs.privateBrowsingId > 0) {
>+      let uri = Services.io.newURI(url, null, null);
>+      let loadingPrincipal = Services.scriptSecurityManager.createCodebasePrincipal(uri, attrs);
>+
>+      channelOptions.loadingPrincipal = loadingPrincipal;
>+    } else {
>+      channelOptions.loadUsingSystemPrincipal = true;
>+    }
>   }
> 
>   try {
>diff --git a/docshell/base/LoadContext.cpp b/docshell/base/LoadContext.cpp
>index 6578bc4..da8a627 100644
>--- a/docshell/base/LoadContext.cpp
>+++ b/docshell/base/LoadContext.cpp
>@@ -53,7 +53,7 @@ LoadContext::LoadContext(nsIPrincipal* aPrincipal,
> {
>   PrincipalOriginAttributes poa = BasePrincipal::Cast(aPrincipal)->OriginAttributesRef();
>   mOriginAttributes.InheritFromDocToChildDocShell(poa);
>-  mOriginAttributes.SyncAttributesWithPrivateBrowsing(mUsePrivateBrowsing);
>+
>   if (!aOptionalBase) {
>     return;
>   }
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>index d349e66..55f721f 100644
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -10774,10 +10774,6 @@ nsDocShell::DoURILoad(nsIURI* aURI,
>     securityFlags |= nsILoadInfo::SEC_SANDBOXED;
>   }
> 
>-  if (UsePrivateBrowsing()) {
>-    securityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;
>-  }
>-
>   nsCOMPtr<nsILoadInfo> loadInfo =
>     (aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT) ?
>       new LoadInfo(loadingWindow, triggeringPrincipal,
>diff --git a/dom/html/HTMLMediaElement.cpp b/dom/html/HTMLMediaElement.cpp
>index c8c773d..67733a0 100644
>--- a/dom/html/HTMLMediaElement.cpp
>+++ b/dom/html/HTMLMediaElement.cpp
>@@ -1321,16 +1321,6 @@ nsresult HTMLMediaElement::LoadResource()
>   nsContentPolicyType contentPolicyType = IsHTMLElement(nsGkAtoms::audio) ?
>     nsIContentPolicy::TYPE_INTERNAL_AUDIO : nsIContentPolicy::TYPE_INTERNAL_VIDEO;
> 
>-  nsDocShell* docShellPtr;
>-  if (docShell) {
>-    docShellPtr = nsDocShell::Cast(docShell);
>-    bool privateBrowsing;
>-    docShellPtr->GetUsePrivateBrowsing(&privateBrowsing);
>-    if (privateBrowsing) {
>-      securityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;
>-    }
>-  }
>-
>   nsCOMPtr<nsILoadGroup> loadGroup = GetDocumentLoadGroup();
>   nsCOMPtr<nsIChannel> channel;
>   nsresult rv = NS_NewChannel(getter_AddRefs(channel),
>diff --git a/image/imgLoader.cpp b/image/imgLoader.cpp
>index 31d5c4e..3674b92 100644
>--- a/image/imgLoader.cpp
>+++ b/image/imgLoader.cpp
>@@ -722,8 +722,7 @@ NewImageChannel(nsIChannel** aResult,
>                 nsLoadFlags aLoadFlags,
>                 nsContentPolicyType aPolicyType,
>                 nsIPrincipal* aLoadingPrincipal,
>-                nsISupports* aRequestingContext,
>-                bool aRespectPrivacy)
>+                nsISupports* aRequestingContext)
> {
>   MOZ_ASSERT(aResult);
> 
>@@ -764,10 +763,6 @@ NewImageChannel(nsIChannel** aResult,
>   }
>   securityFlags |= nsILoadInfo::SEC_ALLOW_CHROME;
> 
>-  if (aRespectPrivacy) {
>-    securityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;
>-  }
>-
>   // Note we are calling NS_NewChannelWithTriggeringPrincipal() here with a
>   // node and a principal. This is for things like background images that are
>   // specified by user stylesheets, where the document is being styled, but
>@@ -1656,8 +1651,7 @@ imgLoader::ValidateRequestWithNewChannel(imgRequest* request,
>                          aLoadFlags,
>                          aLoadPolicyType,
>                          aLoadingPrincipal,
>-                         aCX,
>-                         mRespectPrivacy);
>+                         aCX);
>     if (NS_FAILED(rv)) {
>       return false;
>     }
>@@ -2182,8 +2176,7 @@ imgLoader::LoadImage(nsIURI* aURI,
>                          requestFlags,
>                          aContentPolicyType,
>                          aLoadingPrincipal,
>-                         aContext,
>-                         mRespectPrivacy);
>+                         aContext);
>     if (NS_FAILED(rv)) {
>       return NS_ERROR_FAILURE;
>     }
>diff --git a/image/imgLoader.h b/image/imgLoader.h
>index ffd4386..a679e9a 100644
>--- a/image/imgLoader.h
>+++ b/image/imgLoader.h
>@@ -255,9 +255,6 @@ public:
>   /**
>    * Get the Private Browsing image loader instance that is used by gecko code,
>    * creating it if necessary.
>-   *
>-   * The nsIChannel objects that this instance creates are created with the
>-   * nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING flag.
>    */
>   static imgLoader* PrivateBrowsingLoader();
> 
>diff --git a/image/test/unit/test_private_channel.js b/image/test/unit/test_private_channel.js
>index bf20af5..8ab2db3 100644
>--- a/image/test/unit/test_private_channel.js
>+++ b/image/test/unit/test_private_channel.js
>@@ -56,9 +56,6 @@ var gImgPath = 'http://localhost:' + server.identity.primaryPort + '/image.png';
> function setup_chan(path, isPrivate, callback) {
>   var uri = NetUtil.newURI(gImgPath);
>   var securityFlags = Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;
>-  if (isPrivate) {
>-    securityFlags |= Ci.nsILoadInfo.SEC_FORCE_PRIVATE_BROWSING;
>-  }
>   var chan =  NetUtil.newChannel({uri: uri, loadUsingSystemPrincipal: true,
>                                   securityFlags: securityFlags});
>   chan.notificationCallbacks = new NotificationCallbacks(isPrivate);
>diff --git a/netwerk/base/LoadInfo.cpp b/netwerk/base/LoadInfo.cpp
>index 52e0467..47207ab 100644
>--- a/netwerk/base/LoadInfo.cpp
>+++ b/netwerk/base/LoadInfo.cpp
>@@ -162,20 +162,6 @@ LoadInfo::LoadInfo(nsIPrincipal* aLoadingPrincipal,
>       }
>     }
> 
>-  if (!(mSecurityFlags & nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING)) {
>-    if (aLoadingContext) {
>-      nsCOMPtr<nsILoadContext> loadContext =
>-        aLoadingContext->OwnerDoc()->GetLoadContext();
>-      if (loadContext) {
>-        bool usePrivateBrowsing;
>-        nsresult rv = loadContext->GetUsePrivateBrowsing(&usePrivateBrowsing);
>-        if (NS_SUCCEEDED(rv) && usePrivateBrowsing) {
>-          mSecurityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;
>-        }
>-      }
>-    }
>-  }
>-
>   InheritOriginAttributes(mLoadingPrincipal, mOriginAttributes);
> }
> 
>@@ -509,14 +495,6 @@ LoadInfo::GetDontFollowRedirects(bool* aResult)
> }
> 
> NS_IMETHODIMP
>-LoadInfo::GetUsePrivateBrowsing(bool* aUsePrivateBrowsing)
>-{
>-  *aUsePrivateBrowsing = (mSecurityFlags &
>-                          nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING);
>-  return NS_OK;
>-}
>-
>-NS_IMETHODIMP
> LoadInfo::GetExternalContentPolicyType(nsContentPolicyType* aResult)
> {
>   *aResult = nsContentUtils::InternalContentPolicyTypeToExternal(mInternalContentPolicyType);
>diff --git a/netwerk/base/nsILoadInfo.idl b/netwerk/base/nsILoadInfo.idl
>index 7a2d329..d5c31f8 100644
>--- a/netwerk/base/nsILoadInfo.idl
>+++ b/netwerk/base/nsILoadInfo.idl
>@@ -166,21 +166,12 @@ interface nsILoadInfo : nsISupports
>   const unsigned long SEC_DONT_FOLLOW_REDIRECTS = (1<<12);
> 
>   /**
>-   * Force private browsing. Setting this flag the private browsing can be
>-   * enforce even when a loading is not happening in the context of a document.
>-   *
>-   * If the flag is true, even if a document context is present,
>-   * GetUsePrivateBrowsing will always return true.
>-   */
>-  const unsigned long SEC_FORCE_PRIVATE_BROWSING = (1<<13);
>-
>-  /**
>    * The SEC_FORCE_INHERIT_PRINCIPAL flag may be dropped when a load info
>    * object is created.  Specifically, it will be dropped if the SEC_SANDBOXED
>    * flag is also present.  This flag is set if SEC_FORCE_INHERIT_PRINCIPAL was
>    * dropped.
>    */
>-  const unsigned long SEC_FORCE_INHERIT_PRINCIPAL_WAS_DROPPED = (1<<14);
>+  const unsigned long SEC_FORCE_INHERIT_PRINCIPAL_WAS_DROPPED = (1<<13);
> 
>   /**
>    * The loadingPrincipal is the principal that is responsible for the load.
>@@ -308,11 +299,6 @@ interface nsILoadInfo : nsISupports
>   [infallible] readonly attribute boolean aboutBlankInherits;
> 
>   /**
>-   * If usePrivateBrowsing is true, private browsing will be used.
>-   */
>-  [infallible] readonly attribute boolean usePrivateBrowsing;
>-
>-  /**
>    * If allowChrome is true, then use nsIScriptSecurityManager::ALLOW_CHROME
>    * when calling CheckLoadURIWithPrincipal().
>    */
>diff --git a/netwerk/base/nsNetUtil.cpp b/netwerk/base/nsNetUtil.cpp
>index f8492ce..eb2c597 100644
>--- a/netwerk/base/nsNetUtil.cpp
>+++ b/netwerk/base/nsNetUtil.cpp
>@@ -2421,22 +2421,11 @@ NS_CompareLoadInfoAndLoadContext(nsIChannel *aChannel)
>     DocShellOriginAttributes originAttrsLoadContext;
>     loadContext->GetOriginAttributes(originAttrsLoadContext);
> 
>-    bool loadInfoUsePB = false;
>-    rv = loadInfo->GetUsePrivateBrowsing(&loadInfoUsePB);
>-    if (NS_FAILED(rv)) {
>-      return NS_ERROR_UNEXPECTED;
>-    }
>-    bool loadContextUsePB = false;
>-    rv = loadContext->GetUsePrivateBrowsing(&loadContextUsePB);
>-    if (NS_FAILED(rv)) {
>-      return NS_ERROR_UNEXPECTED;
>-    }
>-
>     LOG(("NS_CompareLoadInfoAndLoadContext - loadInfo: %d, %d, %d, %d; "
>          "loadContext: %d %d, %d, %d. [channel=%p]",
>          originAttrsLoadInfo.mAppId, originAttrsLoadInfo.mInIsolatedMozBrowser,
>-         originAttrsLoadInfo.mUserContextId, loadInfoUsePB,
>-         loadContextAppId, loadContextUsePB,
>+         originAttrsLoadInfo.mUserContextId, originAttrsLoadInfo.mPrivateBrowsingId,
>+         loadContextAppId, originAttrsLoadContext.mPrivateBrowsingId,
>          originAttrsLoadContext.mUserContextId, loadContextIsInBE,
>          aChannel));
> 
>@@ -2454,8 +2443,9 @@ NS_CompareLoadInfoAndLoadContext(nsIChannel *aChannel)
>                "The value of mUserContextId in the loadContext and in the "
>                "loadInfo are not the same!");
> 
>-    MOZ_ASSERT(loadInfoUsePB == loadContextUsePB,
>-               "The value of usePrivateBrowsing in the loadContext and in the loadInfo "
>+    MOZ_ASSERT(originAttrsLoadInfo.mPrivateBrowsingId ==
>+               originAttrsLoadContext.mPrivateBrowsingId,
>+               "The value of mPrivateBrowsingId in the loadContext and in the loadInfo "
>                "are not the same!");
>   }
>   return NS_OK;
>diff --git a/netwerk/test/unit/test_cacheflags.js b/netwerk/test/unit/test_cacheflags.js
>index f506381..949d6e3 100644
>--- a/netwerk/test/unit/test_cacheflags.js
>+++ b/netwerk/test/unit/test_cacheflags.js
>@@ -1,5 +1,6 @@
> Cu.import("resource://testing-common/httpd.js");
> Cu.import("resource://gre/modules/NetUtil.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");
> 
> var httpserver = new HttpServer();
> httpserver.start(-1);
>@@ -18,14 +19,16 @@ var test404Path = "/test404" + suffix;
> 
> // We attach this to channel when we want to test Private Browsing mode
> function LoadContext(usePrivateBrowsing) {
>+  this.originAttributes.privateBrowsingId = usePrivateBrowsing ? 1 : 0;
>   this.usePrivateBrowsing = usePrivateBrowsing;
> }
> 
> LoadContext.prototype = {
>-  originAttributes: {},
>-  usePrivateBrowsing: false,
>+  originAttributes: {
>+    privateBrowsingId : 0
>+  },
>+  usePrivateBrowsing : false,
>   // don't bother defining rest of nsILoadContext fields: don't need 'em
>-
>   QueryInterface: function(iid) {
>     if (iid.equals(Ci.nsILoadContext))
>       return this;
>@@ -36,20 +39,22 @@ LoadContext.prototype = {
>     if (iid.equals(Ci.nsILoadContext))
>       return this;
>     throw Cr.NS_ERROR_NO_INTERFACE;
>-  },
>-
>-  originAttributes: {}
>+  }
> };
> 
>-PrivateBrowsingLoadContext = new LoadContext(true);
>+var PrivateBrowsingLoadContext = new LoadContext(true);
> 
> function make_channel(url, flags, usePrivateBrowsing) {
>   var securityFlags = Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;
>-  if (usePrivateBrowsing) {
>-    securityFlags |= Ci.nsILoadInfo.SEC_FORCE_PRIVATE_BROWSING;
>-  }
>-  var req = NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true,
>-                                securityFlags: securityFlags});
>+
>+  var uri = Services.io.newURI(url, null, null);
>+  var principal = Services.scriptSecurityManager.createCodebasePrincipal(uri,
>+    { privateBrowsingId : usePrivateBrowsing ? 1 : 0 });
>+
>+  var req = NetUtil.newChannel({uri: uri,
>+                                loadingPrincipal: principal,
>+                                securityFlags: securityFlags,
>+                                contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER});
>   req.loadFlags = flags;
>   if (usePrivateBrowsing) {
>     req.notificationCallbacks = PrivateBrowsingLoadContext;
>

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8766386 [details] [diff] [review]
Bug1282124.patch

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

r+ ONLY FOR NETWORKING PARTS

this has to be reviewed by someone who maintains loadcontext/loadinfo/originattributes.  maybe Bobby Holley?

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8766386 [details] [diff] [review]
Bug1282124.patch

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

Don't know much about this. Bouncing to sicking.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8766386 [details] [diff] [review]
Bug1282124.patch

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

::: devtools/shared/DevToolsUtils.js
@@ +550,5 @@
> +    if (attrs.privateBrowsingId > 0) {
> +      let uri = Services.io.newURI(url, null, null);
> +      let loadingPrincipal = Services.scriptSecurityManager.createCodebasePrincipal(uri, attrs);
> +
> +      channelOptions.loadingPrincipal = loadingPrincipal;

The loadingPrincipal affects a lot more than just which cookies are used. Are you sure you want all of those changes here?

What you probably want here is to just change the cookie jar which is used. To do that set 
channel.loadInfo.originAttributes = attrs;

::: image/imgLoader.cpp
@@ -2182,5 @@
>                           requestFlags,
>                           aContentPolicyType,
>                           aLoadingPrincipal,
> -                         aContext,
> -                         mRespectPrivacy);

We should probably remove mRespectPrivacy since it doesn't do anything any more, right? Which means that we also should remove the PrivateBrowsingLoader() function?

Comment 11

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

Comment on attachment 8766386 [details] [diff] [review]
Bug1282124.patch

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

::: netwerk/base/nsILoadInfo.idl
@@ -309,5 @@
>  
>    /**
> -   * If usePrivateBrowsing is true, private browsing will be used.
> -   */
> -  [infallible] readonly attribute boolean usePrivateBrowsing;

Please see my comment https://bugzilla.mozilla.org/show_bug.cgi?id=1292450#c14.
I worried that removing this will have some problems

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8766386 [details] [diff] [review]
Bug1282124.patch

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

::: devtools/shared/DevToolsUtils.js
@@ +548,5 @@
> +    }
> +
> +    if (attrs.privateBrowsingId > 0) {
> +      let uri = Services.io.newURI(url, null, null);
> +      let loadingPrincipal = Services.scriptSecurityManager.createCodebasePrincipal(uri, attrs);

If you just want to change cookies here. You should simply leave the loadingPrincipal as-is, but set loadInfo.originAttributes.

But really, rather than signeling out OA.privateBrowsingId, we should probably simply do:

let attrs = undefined;
if (window) {
  attrs = window.document.nodePrincipal.originAttributes;
}

And then, after having created the channel below, do

channel.loadInfo.originAttributes = attrs;


(I'm not 100% sure of the syntax in either of these code snippets, but hopefully the idea comes across?)


Even cooler would be if we add support for a .originAttributes property in NetUtil.newChannel. And then let NetUtil.newChannel set channel.loadInfo.originAttributes after having created the channel when channelOptions.originAttributes is provided.

Comment 13

[(no longer active)]

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #12)
> Comment on attachment 8766386 [details] [diff] [review]
> Bug1282124.patch
> 
> Review of attachment 8766386 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/shared/DevToolsUtils.js
> @@ +548,5 @@
> > +    }
> > +
> > +    if (attrs.privateBrowsingId > 0) {
> > +      let uri = Services.io.newURI(url, null, null);
> > +      let loadingPrincipal = Services.scriptSecurityManager.createCodebasePrincipal(uri, attrs);
> 
> If you just want to change cookies here. You should simply leave the
> loadingPrincipal as-is, but set loadInfo.originAttributes.
> 
> But really, rather than signeling out OA.privateBrowsingId, we should
> probably simply do:
> 
> let attrs = undefined;
> if (window) {
>   attrs = window.document.nodePrincipal.originAttributes;
> }
> 
> And then, after having created the channel below, do
> 
> channel.loadInfo.originAttributes = attrs;

This sounds good to me!

> Even cooler would be if we add support for a .originAttributes property in
> NetUtil.newChannel. And then let NetUtil.newChannel set
> channel.loadInfo.originAttributes after having created the channel when
> channelOptions.originAttributes is provided.

This sounds even better!

Comment 14

[(no longer active)]

I finally got to a patch which passes the try server.  There are some substantial changes in this version of the patch that merit another review, I think.  In particular:

* DevToolsUtils.js parts were rewritten.
* The imgLoader parts were fixed so that we avoid using the system principal to load images.
* test_private_channel.js was changed.
* LoadInfo parts were changed to respect the loading context for non-system principals.
* test_cacheflags.js was changed slightly.

Note that while the two patches look similar, this is a rewrite of the previous patch from scratch, and there were a few changes in the original patch which weren't actually needed.  I recommend reviewing this as a new patch.

Comment 15

[(no longer active)]

Attachment: Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag

Original patch by James Andreou <[e-mail]>.

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8789021 [details] [diff] [review]
Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag

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

Only reviewed DevTools change, which doesn't seem quite right to me yet.

Also, I will be on PTO Sept. 12 - 23.  I would suggest :ochameau as an alternate reviewer while I am away.

::: devtools/shared/DevToolsUtils.js
@@ +513,3 @@
>    try {
> +    uri = Services.io.newURI(url, null, null);
> +  } catch (e if (e.result == Cr.NS_ERROR_MALFORMED_URI)) {

I believe this is SpiderMonkey specific syntax, which we're trying to avoid to support linters and other tools.  Change to `catch (e)` and then move the if inside the catch block.

@@ +523,5 @@
> +    securityFlags: securityFlags,
> +    uri: uri
> +  };
> +  let prin = principal;
> +  if (!prin) {

This doesn't seem quite right to me yet.  Looking at the `mainThreadFetch` function (exported as `fetch`), its comments say the system principal is used if you don't pass one explicitly.  This is typically used because we don't want fetches by the DevTools to load various sources to show up on the network monitor for that tab.

This change currently appears to alter that behavior so that if a principal is not passed (which appears to be the typical usage pattern[1]), we construct the principal from the URL and OA of the window (if we have a window).

So this seems to change at least two things:

1. We no longer use the system principal when the principal is not passed
2. The SEC_FORCE_PRIVATE_BROWSING flag was previous applied if a window was passed, but now it appears you would need to pass a window *and* _not_ pass a principal so that this block copies the window's OA.

It's not immediately obvious to me that these changes are correct, but I also don't have a deep understanding of how all these flags interact in the end.  If you believe it is correct, please help me follow along!

[1]: https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%5Cbfetch%5Cb%5C(+path%3Adevtools&redirect=false

@@ +542,2 @@
>      return NetUtil.newChannel(channelOptions);
> +  } catch (e if (e.result == Cr.NS_ERROR_UNKNOWN_PROTOCOL)) {

Same note as above about catch syntax.

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8789021 [details] [diff] [review]
Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag

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

::: devtools/shared/DevToolsUtils.js
@@ +531,5 @@
> +    }
> +    prin = Services.scriptSecurityManager
> +                   .createCodebasePrincipal(uri, oa);
> +  }
> +  // contentPolicyType is required when loading with a custom principal

this comment is wrong in this scenario, right? The principal here is what has been passed as argument.

Comment 18

[(no longer active)]

Thanks for the review!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> Only reviewed DevTools change, which doesn't seem quite right to me yet.
> 
> Also, I will be on PTO Sept. 12 - 23.  I would suggest :ochameau as an
> alternate reviewer while I am away.
> 
> ::: devtools/shared/DevToolsUtils.js
> @@ +513,3 @@
> >    try {
> > +    uri = Services.io.newURI(url, null, null);
> > +  } catch (e if (e.result == Cr.NS_ERROR_MALFORMED_URI)) {
> 
> I believe this is SpiderMonkey specific syntax, which we're trying to avoid
> to support linters and other tools.  Change to `catch (e)` and then move the
> if inside the catch block.

Will do.

> @@ +523,5 @@
> > +    securityFlags: securityFlags,
> > +    uri: uri
> > +  };
> > +  let prin = principal;
> > +  if (!prin) {
> 
> This doesn't seem quite right to me yet.  Looking at the `mainThreadFetch`
> function (exported as `fetch`), its comments say the system principal is
> used if you don't pass one explicitly.  This is typically used because we
> don't want fetches by the DevTools to load various sources to show up on the
> network monitor for that tab.
> 
> This change currently appears to alter that behavior so that if a principal
> is not passed (which appears to be the typical usage pattern[1]), we
> construct the principal from the URL and OA of the window (if we have a
> window).
> 
> So this seems to change at least two things:
> 
> 1. We no longer use the system principal when the principal is not passed
> 2. The SEC_FORCE_PRIVATE_BROWSING flag was previous applied if a window was
> passed, but now it appears you would need to pass a window *and* _not_ pass
> a principal so that this block copies the window's OA.
> 
> It's not immediately obvious to me that these changes are correct, but I
> also don't have a deep understanding of how all these flags interact in the
> end.  If you believe it is correct, please help me follow along!

OK, let me explain these changes.

Over in bug 1269361, we changed our private browsing implementation to rely on a new flag on OriginAttributes called privateBrowsingId.  For the system principal (which is always a singleton) and also for loads which do not need to honor private browsing, we use 0 for privateBrowsingId, and 1 for loads that do need to respect PB.

In the old days, we were using flags such as SEC_FORCE_PRIVATE_BROWSING to force a load into PB mode because there was simply no information elsewhere that would correlate a load with its PB-ness.  Now, this information is expected to be available on the loading principal.

This is the reason why in cases where we're passed a principal, we can just use that, since the principal's OA will tell us whether we need to honor PB semantics.  (This is also why we can actually remove the SEC_FORCE_PRIVATE_BROWSING flag.)

Also, looking at what principal we use for loads, using the system principal always means that you will never end up honoring PB.  The assumption here is that only loads that aren't related to any content should happen with the same principal.  In case we do need to honor PB (which is the case where the SEC_FORCE_PRIVATE_BROWSING flag is being used with a system principal currently) instead we need to create a codebase principal with the right OA.

Sorry the patch was so confusing; none of this was evident by looking at the patch.  I should have described the setup before asking for review!

Comment 19

[(no longer active)]

Attachment: Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag

Original patch by James Andreou <[e-mail]>.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8789498 [details] [diff] [review]
Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag

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

Thanks for the additional details!  It sounds like this approach will lead to better correctness around preserving the PB status.

I tried some manual testing of various edge cases, and I did not notice any regressions in the tools so far.  Let's move forward with this and we can always tweak again later as needed.

r+ on the DevTools portion.

::: devtools/shared/DevToolsUtils.js
@@ +5,5 @@
>  "use strict";
>  
>  /* General utilities used throughout devtools. */
>  
> +var { Ci, Cu, Cc, Cr, components } = require("chrome");

Seems like you can revert this change since we're no longer testing error types.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8789498 [details] [diff] [review]
Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag

>-  if (!(mSecurityFlags & nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING)) {
>-    if (aLoadingContext) {
>-      nsCOMPtr<nsILoadContext> loadContext =
>-        aLoadingContext->OwnerDoc()->GetLoadContext();
>-      if (loadContext) {
>-        bool usePrivateBrowsing;
>-        nsresult rv = loadContext->GetUsePrivateBrowsing(&usePrivateBrowsing);
>-        if (NS_SUCCEEDED(rv) && usePrivateBrowsing) {
>-          mSecurityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;
>-        }
>-      }
>-    }
>-  }
So this one sets the pb flag even when chrome docshell initiates the load


>-
>   InheritOriginAttributes(mLoadingPrincipal, mOriginAttributes);
> 
>+  // We need to do this after inheriting the document's origin attributes
>+  // above, in case the loading principal ends up being the system principal.
>+  if (aLoadingContext) {
>+    nsCOMPtr<nsILoadContext> loadContext =
>+      aLoadingContext->OwnerDoc()->GetLoadContext();
>+    nsCOMPtr<nsIDocShell> docShell = aLoadingContext->OwnerDoc()->GetDocShell();
>+    if (loadContext && docShell &&
>+        docShell->ItemType() == nsIDocShellTreeItem::typeContent) {
>+      bool usePrivateBrowsing;
>+      nsresult rv = loadContext->GetUsePrivateBrowsing(&usePrivateBrowsing);
>+      if (NS_SUCCEEDED(rv)) {
>+        mOriginAttributes.SyncAttributesWithPrivateBrowsing(usePrivateBrowsing);
>+      }
>+    }
>+  }
But this doesn't.
Why do we want to change the behavior? Please explain and ask review, or somehow keep the old behavior.


IIRC, allstars had the idea that perhaps pb flag should be after all set in chrome docshells' OA too, just not inherit to system principal. But, that might not help at all here.
pb + system principal is a tricky case.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Using the private-browsing cookie jar for chrome code as a default is likely not what we want. If we want chrome to use different cookies than the "normal" web cookies, then wouldn't we also want it to use different cookies than the PB cookies?

Also note that setting OAs in chrome docshells won't really affect much. Cookies are these days derived from nsILoadInfo, which by default inherits the OA from the loadingPrincipal (i.e. the system principal for chrome). Not the docshell. Likewise IndexedDB/localStorage/etc derives the OA from the document principal.

The OA in the docshell only affect the cookies that are used to load the document of the docshell. But for chrome stuff that is the XUL file loaded from chrome://, where we don't send cookies.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #22)
> Using the private-browsing cookie jar for chrome code as a default is likely
> not what we want. 
I think I agree. But that is what we do currently, as far as I see.

Comment 24

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8789498 [details] [diff] [review]
> Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag
> 
> >-  if (!(mSecurityFlags & nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING)) {
> >-    if (aLoadingContext) {
> >-      nsCOMPtr<nsILoadContext> loadContext =
> >-        aLoadingContext->OwnerDoc()->GetLoadContext();
> >-      if (loadContext) {
> >-        bool usePrivateBrowsing;
> >-        nsresult rv = loadContext->GetUsePrivateBrowsing(&usePrivateBrowsing);
> >-        if (NS_SUCCEEDED(rv) && usePrivateBrowsing) {
> >-          mSecurityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;
> >-        }
> >-      }
> >-    }
> >-  }
> So this one sets the pb flag even when chrome docshell initiates the load
> 
> 
> >-
> >   InheritOriginAttributes(mLoadingPrincipal, mOriginAttributes);
> > 
> >+  // We need to do this after inheriting the document's origin attributes
> >+  // above, in case the loading principal ends up being the system principal.
> >+  if (aLoadingContext) {
> >+    nsCOMPtr<nsILoadContext> loadContext =
> >+      aLoadingContext->OwnerDoc()->GetLoadContext();
> >+    nsCOMPtr<nsIDocShell> docShell = aLoadingContext->OwnerDoc()->GetDocShell();
> >+    if (loadContext && docShell &&
> >+        docShell->ItemType() == nsIDocShellTreeItem::typeContent) {
> >+      bool usePrivateBrowsing;
> >+      nsresult rv = loadContext->GetUsePrivateBrowsing(&usePrivateBrowsing);
> >+      if (NS_SUCCEEDED(rv)) {
> >+        mOriginAttributes.SyncAttributesWithPrivateBrowsing(usePrivateBrowsing);
> >+      }
> >+    }
> >+  }
> But this doesn't.
> Why do we want to change the behavior? Please explain and ask review, or
> somehow keep the old behavior.

Yes, this behavior change is quite intentional.  The old behavior is in fact not even desirable for the reasons that Jonas mentioned, we were just sloppy about it here before, but for the most part, code that triggers load in Gecko with the system principal was never subject to PB.

> IIRC, allstars had the idea that perhaps pb flag should be after all set in
> chrome docshells' OA too, just not inherit to system principal. But, that
> might not help at all here.
> pb + system principal is a tricky case.

The general rule of thumb is: system principal implies not PB.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8789498 [details] [diff] [review]
Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag

>@@ -732,24 +728,31 @@ NewImageChannel(nsIChannel** aResult,
>                                               securityFlags,
>                                               aPolicyType,
>                                               nullptr,   // loadGroup
>                                               callbacks,
>                                               aLoadFlags);
>   } else {
>     // either we are loading something inside a document, in which case
>     // we should always have a requestingNode, or we are loading something
>-    // outside a document, in which case the loadingPrincipal and
>-    // triggeringPrincipal should always be the systemPrincipal.
>+    // outside a document, in which case we create a codebase principal using
>+    // the origin attributes reflecting private browsing state.
>     // However, there are two exceptions: one is Notifications and the
>     // other one is Favicons which create a channel in the parent prcoess
>     // in which case we can't get a requestingNode.
>+    PrincipalOriginAttributes attrs;
>+    if (aLoadingPrincipal) {
>+      attrs = BasePrincipal::Cast(aLoadingPrincipal)->OriginAttributesRef();
>+    }
>+    attrs.mPrivateBrowsingId = aRespectPrivacy ? 1 : 0;
>+    nsCOMPtr<nsIPrincipal> principal =
>+      BasePrincipal::CreateCodebasePrincipal(aURI, attrs);
>     rv = NS_NewChannel(aResult,
>                        aURI,
>-                       nsContentUtils::GetSystemPrincipal(),
>+                       principal,
>                        securityFlags,
>                        aPolicyType,
>                        nullptr,   // loadGroup
>                        callbacks,
>                        aLoadFlags);
Could you explain why we need to create a new principal here and can't just reuse loading principal?
This feels like cheating (though, passing system principal has been equally cheating).
If loading principal has the right OA, why can't we use the whole principal here?

Can't really r+, since I don't understand why we have this seemingly broken or at least unusual setup in image loader.
Please explain and ask review again, or fix to use loading principal if possible.

Christoph, perhaps you could explain the behavior? The systemprincipal usage is from https://bugzilla.mozilla.org/show_bug.cgi?id=1206961

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8789498 [details] [diff] [review]
Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag

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

::: image/imgLoader.cpp
@@ +748,3 @@
>      rv = NS_NewChannel(aResult,
>                         aURI,
> +                       principal,

Using the SystemPrincipal is most definitely correct here and we should not change that. I think the problem you are facing is that you can't assign custom origintattributes to the SystemPrincipal (because there is only one instance of it), right?

What you can do though is overwrite the originAttributes on the loadInfo [1]. What I suggest is:
(a) Create the channel (using systemPrincipal) as is.
(b) Query the loadInfo from the newly created channel
(c) Set custom origin attributes on that loadInfo

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#683

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Why using system principal is right here? 
(and you're @tpac? haven't seen you yet :) )

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] (TPAC) from comment #27)
> Why using system principal is right here? 

I suppose we should always have a requestingNode in case case we are loading something inside a document, in that case the |if (requestingNode && aLoadingPrincipal) {| branch is entered and we are creating a channel calling NS_NewChannelWithTriggeringPrincipal. If that is not the case, we are loading something outside a document in which case the systemPrincipal should be correct to use. Looking at the comment, it mentions two exeptions to that rule: Favicons and Notifications which create a channel in the parent process. Since we are not serializing Nodes between child and parent, I suppose that's acceptable. I also think we are working on using the right principal for favicons (See Bug 1277803).

Btw, creating a codeBasePrincipal using the uri we are about to load (aURI in that case) is almost certainly wrong in all the cases, because creating a principal using the same uri we are about to load basically does not perform meaningful security checks.

> (and you're @tpac? haven't seen you yet :) )
You'll see me around :-)

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Does using system principal here perform any more meaningful security checks than codeBasePrincipal?

Comment 30

[(no longer active)]

Yeah, the reason I used a codebase principal is to *bypass* security checks, which is exactly what the system principal does, right?  I would be happy to make the change you suggest in comment 26, but I'd like to understand why.  :-)

(Also note that favicons don't actually go through this path, since they're loaded in a XUL document and will have a requestingNode that has the system principal (since it's a <xul:image>), so bug 1277803 won't affect this.  See attachment 8790581 [details] [diff] [review] where the other path is changed.)

Comment 31

[(no longer active)]

(In reply to Olli Pettay [:smaug] (TPAC) from comment #25)
> Comment on attachment 8789498 [details] [diff] [review]
> Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag
> 
> >@@ -732,24 +728,31 @@ NewImageChannel(nsIChannel** aResult,
> >                                               securityFlags,
> >                                               aPolicyType,
> >                                               nullptr,   // loadGroup
> >                                               callbacks,
> >                                               aLoadFlags);
> >   } else {
> >     // either we are loading something inside a document, in which case
> >     // we should always have a requestingNode, or we are loading something
> >-    // outside a document, in which case the loadingPrincipal and
> >-    // triggeringPrincipal should always be the systemPrincipal.
> >+    // outside a document, in which case we create a codebase principal using
> >+    // the origin attributes reflecting private browsing state.
> >     // However, there are two exceptions: one is Notifications and the
> >     // other one is Favicons which create a channel in the parent prcoess
> >     // in which case we can't get a requestingNode.
> >+    PrincipalOriginAttributes attrs;
> >+    if (aLoadingPrincipal) {
> >+      attrs = BasePrincipal::Cast(aLoadingPrincipal)->OriginAttributesRef();
> >+    }
> >+    attrs.mPrivateBrowsingId = aRespectPrivacy ? 1 : 0;
> >+    nsCOMPtr<nsIPrincipal> principal =
> >+      BasePrincipal::CreateCodebasePrincipal(aURI, attrs);
> >     rv = NS_NewChannel(aResult,
> >                        aURI,
> >-                       nsContentUtils::GetSystemPrincipal(),
> >+                       principal,
> >                        securityFlags,
> >                        aPolicyType,
> >                        nullptr,   // loadGroup
> >                        callbacks,
> >                        aLoadFlags);
> Could you explain why we need to create a new principal here and can't just
> reuse loading principal?

Because the loading principal can be null where the caller means "use the system principal", which is the case this patch is trying to solve.  :-)

> Can't really r+, since I don't understand why we have this seemingly broken
> or at least unusual setup in image loader.
> Please explain and ask review again, or fix to use loading principal if
> possible.

I'm not sure which part of the setup you're objecting to.  Again, the core issue is that we may have a system principal on our hands here, and we can't modify its OA, so we need to either do what my patch is doing or what Christoph is suggesting, but I prefer avoiding overloading the loadinfo's OAs in cases where we can.

This patch shouldn't check any security checks or any other behavior in the image loader.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Do redirects cause some security checks with this setup which might cause issues?

Generating random CreateCodebasePrincipals feels rather wrong to me. If we want to always load, just use systemprincipal and pass OA

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Ehsan Akhgari from comment #30)
> Yeah, the reason I used a codebase principal is to *bypass* security checks,
> which is exactly what the system principal does, right?  I would be happy to
> make the change you suggest in comment 26, but I'd like to understand why. 
> :-)

I understand your point, but semantically it feels different if we query the loadingPrincipal (or also TriggeringPrincipal) at a random point during execution and we get back a codebaseprincipal instead of a systemPrincipal from the loadinfo. I also think it might cause some unexpected behavior. E.g. using the systemPrincipal does not cause us to actually set up CORS but using a codebaseprincipal does, e.g. here [1] which might cause unexpected behavior and hence prevent certain images from loading. Maybe not in the initial loading, but it could cause problems after a redirect as Olli mentionend.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#145

(In reply to :Ehsan Akhgari from comment #31)
> I'm not sure which part of the setup you're objecting to.  Again, the core
> issue is that we may have a system principal on our hands here, and we can't
> modify its OA, so we need to either do what my patch is doing or what
> Christoph is suggesting, but I prefer avoiding overloading the loadinfo's
> OAs in cases where we can.

Why is overloading the OA in the loadinfo such bad practice? Or put differently, what's the downside? I thought we do that in lots of places in the codebase where we have to use the systemPrincipal but actually want different OA?

Comment 34

[(no longer active)]

The reason to avoid overriding the OA on the loadinfo is that in most cases the correct OA must be obtained from the principal, except of course for the system principal in weird cases where the load isn't happening "on behalf of the system" and thus needs to diverge in its OA from the principal.

At any rate, we have discussed this to death.  I'll use the system principal.

Comment 35

[(no longer active)]

Attachment: Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag

Original patch by James Andreou <[e-mail]>.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Using the system principal means that *no* security checks will be done. Including no calls to nsIContentPolicy.

Using a codebase principal means that we will do some security checks, even when the loaded URI is same-origin with the loadingPrincipal. Specifically we will call nsIContentPolicy and likely will call into web-extensions.

Additionally, there are redirects to consider, which obviously will behave differently for codebase and system principal as loadingPrincipal.

So if you really want to avoid security checks, then use a system principal.

OTOH, if a webpage is able to control the URI which is being loaded, then most likely you should not use a system principal to do the load.

Comment 37

[(no longer active)]

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #36)
> OTOH, if a webpage is able to control the URI which is being loaded, then
> most likely you should not use a system principal to do the load.

This code path is used for internal image loads, so I don't think this is a concern here.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c6da3081bf
Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag; r=smaug,jryans

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31c6da3081bf

Comment 40

[Tanvi Vyas[:tanvi]]

Given https://bugzilla.mozilla.org/show_bug.cgi?id=1319908, can we back this out?

Comment 41

[(no longer active)]

No, we should just fix that bug.