Tanvi Vyas[:tanvi] Reporter
	Description • 8 years ago

Dave already has a patch in bug 962365, so if that lands before this does, we can close this.

+++ This bug was initially created as a clone of Bug #962365 +++

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 1 • 8 years ago

Rebased the patch from bug 962365. Need to resolve test failure next.

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 2 • 8 years ago

Does this patch handle upgrade and downgrade of profiles? For example, if we land this in nightly, and then someone opens the profile in nightly - will the existing cache be lost or cause issues somehow? Also vice versa when they take the profile back to release version and the cache now contains extra values. Probably not the end of the world if these cache entries are purged, but we'd still need to handle it, right? Or does the patch handle this already?

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 3 • 8 years ago

(In reply to Paul Theriault [:pauljt] from comment #2)
> Does this patch handle upgrade and downgrade of profiles? For example, if we
> land this in nightly, and then someone opens the profile in nightly - will
> the existing cache be lost or cause issues somehow? Also vice versa when
> they take the profile back to release version and the cache now contains
> extra values. Probably not the end of the world if these cache entries are
> purged, but we'd still need to handle it, right? Or does the patch handle
> this already?

I don't think we need to do anything special about upgrade and downgrade.

If someone uses a nightly version with image cache double-key, they can still access their old cache in default user context because the origin suffix will be empty, so the cache key remains the same.

We need not purge these cache entries because they won't be accessed in a release without image cache double-key, and we shouldn't because the user might still use nightly again sometime.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 4 • 8 years ago

About the test failure on test_private_channel.js, two things are tested: LoadImageWithChannelXPCOM and LoadImageXPCOM. Between them the cache is cleared. If I test only one of them, it is fine, so I think the reason is the cache isn't cleared correctly.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 5 • 8 years ago

When I applied Dave's patch, I found that these four LoadImage calls[1] all got "cache hit"[2]. I thought the reason was the cache wasn't cleared properly. 

So I turned on the "imgRequest" log module and popped out Dave's patch, trying to look at the original behavior. I found that those four calls all got "cache miss"[3]. Then I don't understand why without Dave's patch, the test would pass. I thought four cache misses would lead to gHit being 4 instead of 2.

Without Dave's patch, the image cache entry couldn't pass a ValidateSecurityInfo() check, inside which it was blocked by a nsMixedContentBlocker::ShouldLoad() check. And because Dave passes system principal to LoadImage, the code path in NS_CheckContentLoadPolicy changed so it passed the ValidateSecurityInfo() check.

Also, Tim told me he also found something wrong with image cache clearing when he was dealing with bug 1238183, so I suspect that image cache clearing already has some problem.

[1] https://dxr.mozilla.org/mozilla-central/source/image/test/unit/test_private_channel.js#95-98
[2] https://dxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#2225
[3] https://dxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#2159

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 6 • 8 years ago

I also tried commenting out the four LoadImageWithChannel calls. If I applied Dave's patch, the latter four LoadImage calls will get a much more normal result: cache miss -> cache hit -> cache miss -> cache hit. But, if I doesn't apply his patch, I got four cache misses again.

Hi Josh,

Please take a look at Comment 5. Is it right that the four LoadImage calls always got cache misses? If so, how is gHits still 2 instead of 4?
Thank you.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 7 • 8 years ago

I cleared the cache with these two lines of code:

> gPrivateLoader.QueryInterface(Ci.imgICache).clearCache(false);
> gPublicLoader.QueryInterface(Ci.imgICache).clearCache(false);

And test_private_channel.js will pass now.

It still bugs me why the test would pass without clearing image caches before applying these changes.

Hi Seth,

May I ask you to review this patch?

Meanwhile I'll write a test to make sure that image cache is separated by origin attributes with this patch applied. Although Tim has already confirmed that it is with his test on ForgetAboutSite.jsm.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 8 • 8 years ago

A test to make sure image cache respect originAttributes.

Before we double-key the image cache, we would hit image cache set in another user context, so this test would fail. Now, the same image will have different image cache keys in different user contexts, so this test will pass.

Hi Seth, would you please also review this patch?

Thanks.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 9 • 8 years ago

Comment on attachment 8754730 [details] [diff] [review]
Double-key the image cache by origin attribute

Hi Josh,

May I ask you to review these patches?

	Josh Matthews [:jdm]
	Comment 10 • 8 years ago

Comment on attachment 8754730 [details] [diff] [review]
Double-key the image cache by origin attribute

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

This generally looks ok to me; we'll definitely want seth's feedback on it, and we'll need to address the addon compatibility risk as well.

::: dom/base/nsContentUtils.cpp
@@ +3120,5 @@
> +    channel->GetLoadInfo(getter_AddRefs(loadInfo));
> +    nsCOMPtr<nsIPrincipal> principal;
> +    NS_ENSURE_TRUE(loadInfo, false);
> +    loadInfo->GetLoadingPrincipal(getter_AddRefs(principal));
> +    NS_ENSURE_TRUE(principal, false);

I'm not convinced that this is the best way to the get principal we want here - in other callers we use the document's node principal instead.

::: image/imgICache.idl
@@ +47,5 @@
>     * @param doc Optional pointer to the document that the cache entry belongs to.
>     * @returns NULL if the URL was not found in the cache
>     */
> +  nsIProperties findEntryProperties(in nsIURI uri,
> +                                    in nsIPrincipal principal,

Almost every caller in this patch is using the document's node principal as the argument; why can't remove this argument and use that internally? Additionally, this is an addon compatibility hazard, as findEntryProperties appears to be in use by a surprising number of addons: https://mxr.mozilla.org/addons/search?string=findentryproperties

::: image/imgLoader.cpp
@@ +1357,5 @@
>                                 nsIDOMDocument* aDOMDoc,
>                                 nsIProperties** _retval)
>  {
>    *_retval = nullptr;
> +  NS_ENSURE_TRUE(principal, NS_ERROR_FAILURE);

I propose defaulting to the system principal if there's no document provided from which to derive a principal so as not to break addons.

::: layout/generic/nsImageFrame.cpp
@@ +2194,5 @@
> +  nsCOMPtr<nsIScriptSecurityManager> ssm = nsContentUtils::GetSecurityManager();
> +  NS_ENSURE_TRUE(ssm, NS_ERROR_FAILURE);
> +  nsCOMPtr<nsIPrincipal> systemPrincipal;
> +  ssm->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
> +  NS_ENSURE_TRUE(systemPrincipal, NS_ERROR_FAILURE);

This isn't used...

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 11 • 8 years ago

Thank you, Josh.

I did what you suggested, keeping the interface of findEntryProperties the same. If document is passed in then we used the node principal; otherwise we use system principal.

	Josh Matthews [:jdm]
	Comment 12 • 8 years ago

Comment on attachment 8757855 [details] [diff] [review]
Double-key the image cache by origin attributes.

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

::: image/imgICache.idl
@@ +10,5 @@
>  interface nsIDocument;
>  interface nsIDOMDocument;
>  interface nsIProperties;
>  interface nsIURI;
> +interface nsIPrincipal;

Don't need this anymore.

	Seth Fowler [:seth] [:s2h]
	Comment 14 • 8 years ago

Comment on attachment 8755359 [details] [diff] [review]
Make sure image cache respect originAttributes

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

Looks good!

::: browser/components/contextualidentity/test/browser/browser_imageCache.js
@@ +54,5 @@
> +  for (let userContextId = 0; userContextId < NUM_USER_CONTEXTS; userContextId++) {
> +    let tab = yield* openTabInUserContext(FILE_URI, userContextId);
> +    gBrowser.removeTab(tab);
> +  }
> +  is(gHits, 3, "should get three image requests");

This hardcoded "3" should really be NUM_USER_CONTEXTS, right? Please use NUM_USER_CONTEXTS instead. Also modify the assertion text to explain this better.

	Seth Fowler [:seth] [:s2h]
	Comment 15 • 8 years ago

Comment on attachment 8758104 [details] [diff] [review]
Double-key the image cache by origin attribute.

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

This all looks fine to me, although I have a lot of style nits. Please also change the commit message so it reads |r=jdm,seth|. You may need to do that for the other patch too.

Please note that I am *not* a security expert, so I'm reviewing this purely from the perspective of the ImageLib code.

::: image/ImageCacheKey.cpp
@@ +103,5 @@
>    // Don't share the image cache between a controlled document and anything else.
>    if (mControlledDocument != aOther.mControlledDocument) {
>      return false;
>    }
> +  // The origin attributes always have to match

Please put a period at the end of this comment.

@@ +135,5 @@
>    // ImageCacheKey, as an optimization we compute our hash once and store it.
>  
>    nsPrintfCString ptr("%p", aControlledDocument);
> +  nsAutoCString suffix;
> +  aAttrs.CreateSuffix(suffix);

Please add a blank line after this line. This block of code is getting pretty big.

::: image/ImageCacheKey.h
@@ +10,5 @@
>  #ifndef mozilla_image_src_ImageCacheKey_h
>  #define mozilla_image_src_ImageCacheKey_h
>  
>  #include "mozilla/Maybe.h"
> +#include "mozilla/BasePrincipal.h"

Wrong alphabetic order here; please fix.

@@ +56,5 @@
>  private:
>    static uint32_t ComputeHash(ImageURL* aURI,
>                                const Maybe<uint64_t>& aBlobSerial,
> +                              void* aControlledDocument,
> +                              const PrincipalOriginAttributes& aAttrs);

Please keep the arguments in the same order between the constructor and ComputeHash(). So make |aAttrs| come before |aControlledDocument|.

@@ +64,5 @@
>    Maybe<uint64_t> mBlobSerial;
>    void* mControlledDocument;
>    uint32_t mHash;
>    bool mIsChrome;
> +  PrincipalOriginAttributes mOriginAttributes;

Same here; move |mOriginAttributes| before |mControlledDocument|. This just makes things a bit easier to read.

::: image/imgLoader.cpp
@@ +2127,5 @@
>    // XXX For now ignore aCacheKey. We will need it in the future
>    // for correctly dealing with image load requests that are a result
>    // of post data.
> +  NS_ENSURE_TRUE(aLoadingPrincipal, NS_ERROR_FAILURE);
> +  PrincipalOriginAttributes attrs =

Shouldn't |attrs| be a |const&|? You're doing a copy here, and you'll do another copy when you pass it to ImageCacheKey. Seems unnecessary.

@@ +2339,5 @@
> +  NS_ENSURE_TRUE(channel, NS_ERROR_FAILURE);
> +  nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();
> +
> +  nsCOMPtr<nsIPrincipal> principal;
> +  NS_ENSURE_TRUE(loadInfo, NS_ERROR_FAILURE);

I think this'd be cleaner if you did it like this:

nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();
NS_ENSURE_TRUE(loadInfo, NS_ERROR_FAILURE);

Same for the other cases below.

::: image/test/unit/async_load_tests.js
@@ +101,5 @@
>    var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
>                  .createScriptedObserver(listener);
> +  var systemPrincipal = ssm.getSystemPrincipal();
> +  requests.push(gCurrentLoader.loadImageXPCOM(uri, null, null, "default",
> +        systemPrincipal, null, outer, null, 0, null));

|systemPrincipal| is an argument to |loadImageXPCOM|. Please align |systemPrincipal| with |uri| on the previous line.

@@ +183,5 @@
>      var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
>                    .createScriptedObserver(listener2);
> +    var systemPrincipal = ssm.getSystemPrincipal();
> +    requests.push(gCurrentLoader.loadImageXPCOM(uri, null, null, "default",
> +          systemPrincipal, null, outer, null, 0, null));

Same here.

@@ +212,5 @@
>    var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
>                  .createScriptedObserver(listener);
> +  var systemPrincipal = ssm.getSystemPrincipal();
> +  var req = gCurrentLoader.loadImageXPCOM(uri, null, null, "default",
> +      systemPrincipal, null, outer, null, 0, null);

Same here.

::: image/test/unit/test_private_channel.js
@@ +87,5 @@
>    loadGroup.notificationCallbacks = new NotificationCallbacks(isPrivate);
>    var loader = isPrivate ? gPrivateLoader : gPublicLoader;
> +  var systemPrincipal = ssm.getSystemPrincipal();
> +  requests.push(loader.loadImageXPCOM(uri, null, null, "default",
> +        systemPrincipal, loadGroup, outer, null, 0, null));

Please align |systemPrincipal| with |uri|.

::: toolkit/system/gnome/nsAlertsIconListener.cpp
@@ +15,5 @@
>  #include "nsIStringBundle.h"
>  #include "nsIObserverService.h"
>  #include "nsIURI.h"
>  #include "nsCRT.h"
> +#include "nsContentUtils.h"

Please alphabetize this #include line correctly. (I realize things aren't in the right order right now, but please put it somewhere more plausible.)

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 16 • 8 years ago

Thanks so much to Josh and Seth.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 18 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0184ab2a470

I think I broke this:
TEST-UNEXPECTED-FAIL | toolkit/content/tests/browser/browser_default_image_filename.js | undefined assertion name - Got index, expected index.gif

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 19 • 8 years ago

MozReview-Commit-ID: AKmMZe0oNT2

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 20 • 8 years ago

(In reply to Jonathan Hao [:jhao] from comment #18)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0184ab2a470
> 
> I think I broke this:
> TEST-UNEXPECTED-FAIL |
> toolkit/content/tests/browser/browser_default_image_filename.js | undefined
> assertion name - Got index, expected index.gif

This test fails because the channel in LoadImageWithChannel has no loading principal. I consulted Christoph on IRC. He suggested that I use default origin attributes in this patch. However, calling LoadImageWithChannel using top-level channel is still odd. I'll open another to track this issue.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=754dc5053896

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 21 • 8 years ago

Comment on attachment 8758536 [details] [diff] [review]
Part 1: Double-key the image cache by origin attribute.

It's not exactly right to use default origin attributes. The fix will need to be reviewed, so I'll put in another patch.

This patch was r+ by seth and jdm.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 22 • 8 years ago

Fix an error in nsAlertsIconListener I made when rebasing.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 23 • 8 years ago

Loading principal could be null if this is a top-level load, so we use triggering principal instead.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 24 • 8 years ago

I found that the loading principal is null in this test, too.
https://dxr.mozilla.org/mozilla-central/source/layout/generic/test/test_bug735641.html

	Christoph Kerschbaumer [:ckerschb]
	Comment 25 • 8 years ago

Comment on attachment 8759038 [details] [diff] [review]
Part 3 - Use triggering principal when loading principal is null.

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

I suppose this patch became obsolete after our dicusssions over IRC, right?

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 26 • 8 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)
> Comment on attachment 8759038 [details] [diff] [review]
> Part 3 - Use triggering principal when loading principal is null.
> 
> Review of attachment 8759038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I suppose this patch became obsolete after our dicusssions over IRC, right?

Yes, it is.

I did some experiment regarding triggering principal. Load this page http://people.mozilla.org/~jhao/image_cache.html in userContext A and right click the link in the page and open in a new container tab in userContext B. I found that the triggering principal's userContextId will be A only if A == B && A != 0. So triggering principals are still problematic.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 27 • 8 years ago

Quoting Christoph: For toplevel loads (TYPE_DOCUMENT) we set the outerwindow within the loadInfo - http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10743
and then I would imagine that those mOriginAtrtributes are correct: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#171

I tested in the same condition as Comment 26. In every combination I can get the correct userContextId.

	Christoph Kerschbaumer [:ckerschb]
	Comment 28 • 8 years ago

Comment on attachment 8759135 [details] [diff] [review]
Part 3 - Get origin attributes from loadInfo when the load is a top-level document.

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

Please add a comment why we need the special case handling for TYPE_DOCUMENT. The change looks good to me, but someone who knows more about originAttributes should review the patch. Probably huseby or tanvi.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 29 • 8 years ago

Thank you, Christoph.

Tanvi, please take a look. Thanks a lot.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 30 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6dc07751dd1

	Tanvi Vyas[:tanvi] Reporter
	Comment 31 • 8 years ago

(In reply to Jonathan Hao [:jhao] from comment #26)
> I did some experiment regarding triggering principal. Load this page
> http://people.mozilla.org/~jhao/image_cache.html in userContext A and right
> click the link in the page and open in a new container tab in userContext B.
> I found that the triggering principal's userContextId will be A only if A ==
> B && A != 0. So triggering principals are still problematic.

What was the triggeringPrincipal when A != B?

Also, are you saying that if A=0 and B=0 (i.e. we are using the default context and not using containers), then the triggeringPrincipal has a nonzero userContextId?  Or that the triggeringPrincipal is system and hence has no userContextId?  Or something else?

Thanks for checking with a testcase!

	Tanvi Vyas[:tanvi] Reporter
	Comment 32 • 8 years ago

Comment on attachment 8759151 [details] [diff] [review]
Part 3 - Get origin attributes from loadInfo when the load is a top-level document.

Some nits below, but otherwise this looks good!  Thanks Jonathan and Christoph!

>diff --git a/image/imgLoader.cpp b/image/imgLoader.cpp
>@@ -2337,22 +2337,30 @@ imgLoader::LoadImageWithChannel(nsIChannel* channel,
>-  nsCOMPtr<nsIPrincipal> principal;
>-  loadInfo->GetLoadingPrincipal(getter_AddRefs(principal));
>-  NS_ENSURE_TRUE(principal, NS_ERROR_FAILURE);
>+  PrincipalOriginAttributes attrs;
>+  // If contentPolicyType == TYPE_DOCUMENT, loading principal could be null.
loadingPrincipal is null.

>+  // This happens when we window.open an image url. 
"This can happen when we window.open a data URI, or when we load an image directly into the top level document."  (jhao - please confirm the latter with a load to something like http://people.mozilla.org/~tvyas/redminus.jpg)

> In this case, when the
In the TYPE_DOCUMENT case, when the

>+  // loadInfo was created, we must've passed the window to its constructor.
>+  // Thus, the loadInfo must have correct origin attributes.
Thus, we can get the originAttributes from the window.

>+  if (loadInfo->InternalContentPolicyType() == nsIContentPolicy::TYPE_DOCUMENT) {
>+    attrs.InheritFromNecko(loadInfo->GetOriginAttributes());
>+  } else {
>+    nsCOMPtr<nsIPrincipal> principal;
>+    loadInfo->GetLoadingPrincipal(getter_AddRefs(principal));
>+    NS_ENSURE_TRUE(principal, NS_ERROR_FAILURE);
> 
>-  const PrincipalOriginAttributes& attrs =
>-    BasePrincipal::Cast(principal)->OriginAttributesRef();
>+    attrs = BasePrincipal::Cast(principal)->OriginAttributesRef();
>+  }
We may be able to do attrs.InheritFromNecko(loadInfo->GetOriginAttributes()); for all content types and remove the if/else.  Since in the non-TYPE_DOCUMENT case, we set mOriginAttributes from mLoadingPrincipal https://mxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#183 and loadInfo->GetOriginAttributes() returns mOriginAttributes.  But I'm not 100% sure about this.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 33 • 8 years ago

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #31)
> (In reply to Jonathan Hao [:jhao] from comment #26)
> > I did some experiment regarding triggering principal. Load this page
> > http://people.mozilla.org/~jhao/image_cache.html in userContext A and right
> > click the link in the page and open in a new container tab in userContext B.
> > I found that the triggering principal's userContextId will be A only if A ==
> > B && A != 0. So triggering principals are still problematic.
> 
> What was the triggeringPrincipal when A != B?
> 
> Also, are you saying that if A=0 and B=0 (i.e. we are using the default
> context and not using containers), then the triggeringPrincipal has a
> nonzero userContextId?  Or that the triggeringPrincipal is system and hence
> has no userContextId?  Or something else?
> 
> Thanks for checking with a testcase!

Sorry, I meant that the triggering principal's userContextId is always 0 except when A == B and A != 0.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 34 • 8 years ago

Thanks, Tanvi. I fixed the nits.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 35 • 8 years ago

This version gets origin attributes from loadInfo regardless of the content type.

Here's the try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21fe1d4009be

It looks good. Tanvi, do you think we should do it this way?

	Tanvi Vyas[:tanvi] Reporter
	Comment 36 • 8 years ago

(In reply to Jonathan Hao (vacation until 6/12) [:jhao] from comment #33)
> (In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #31)
> > (In reply to Jonathan Hao [:jhao] from comment #26)
> > > I did some experiment regarding triggering principal. Load this page
> > > http://people.mozilla.org/~jhao/image_cache.html in userContext A and right
> > > click the link in the page and open in a new container tab in userContext B.
> > > I found that the triggering principal's userContextId will be A only if A ==
> > > B && A != 0. So triggering principals are still problematic.
> > 
> > What was the triggeringPrincipal when A != B?
> > 
> > Also, are you saying that if A=0 and B=0 (i.e. we are using the default
> > context and not using containers), then the triggeringPrincipal has a
> > nonzero userContextId?  Or that the triggeringPrincipal is system and hence
> > has no userContextId?  Or something else?
> > 
> > Thanks for checking with a testcase!
> 
> Sorry, I meant that the triggering principal's userContextId is always 0
> except when A == B and A != 0.

So assume A != 0 and B != 0, then what happens?
Ex: Go to http://people.mozilla.org/~jhao/image_cache.html in the Personal Context (this is A).
Right click the link and select Open New Container Tab->Work (this is B).

What userContextID does the triggeringPrincipal have?  And is a referrer sent during the load in the B?

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 37 • 8 years ago

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #36)
> (In reply to Jonathan Hao (vacation until 6/12) [:jhao] from comment #33)
> > (In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #31)
> > > (In reply to Jonathan Hao [:jhao] from comment #26)
> > > > I did some experiment regarding triggering principal. Load this page
> > > > http://people.mozilla.org/~jhao/image_cache.html in userContext A and right
> > > > click the link in the page and open in a new container tab in userContext B.
> > > > I found that the triggering principal's userContextId will be A only if A ==
> > > > B && A != 0. So triggering principals are still problematic.
> > > 
> > > What was the triggeringPrincipal when A != B?
> > > 
> > > Also, are you saying that if A=0 and B=0 (i.e. we are using the default
> > > context and not using containers), then the triggeringPrincipal has a
> > > nonzero userContextId?  Or that the triggeringPrincipal is system and hence
> > > has no userContextId?  Or something else?
> > > 
> > > Thanks for checking with a testcase!
> > 
> > Sorry, I meant that the triggering principal's userContextId is always 0
> > except when A == B and A != 0.
> 
> So assume A != 0 and B != 0, then what happens?
> Ex: Go to http://people.mozilla.org/~jhao/image_cache.html in the Personal
> Context (this is A).
> Right click the link and select Open New Container Tab->Work (this is B).
> 
> What userContextID does the triggeringPrincipal have?  And is a referrer
> sent during the load in the B?

userContextId is 0, I forgot to check the referrer.

	Tanvi Vyas[:tanvi] Reporter
	Comment 38 • 8 years ago

(In reply to Jonathan Hao (vacation until 6/12) [:jhao] from comment #37)
> (In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #36)
> > (In reply to Jonathan Hao (vacation until 6/12) [:jhao] from comment #33)
> > > (In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #31)
> > > > (In reply to Jonathan Hao [:jhao] from comment #26)
> > > > > I did some experiment regarding triggering principal. Load this page
> > > > > http://people.mozilla.org/~jhao/image_cache.html in userContext A and right
> > > > > click the link in the page and open in a new container tab in userContext B.
> > > > > I found that the triggering principal's userContextId will be A only if A ==
> > > > > B && A != 0. So triggering principals are still problematic.
> > > > 
> > > > What was the triggeringPrincipal when A != B?
> > > > 
> > > > Also, are you saying that if A=0 and B=0 (i.e. we are using the default
> > > > context and not using containers), then the triggeringPrincipal has a
> > > > nonzero userContextId?  Or that the triggeringPrincipal is system and hence
> > > > has no userContextId?  Or something else?
> > > > 
> > > > Thanks for checking with a testcase!
> > > 
> > > Sorry, I meant that the triggering principal's userContextId is always 0
> > > except when A == B and A != 0.
> > 
> > So assume A != 0 and B != 0, then what happens?
> > Ex: Go to http://people.mozilla.org/~jhao/image_cache.html in the Personal
> > Context (this is A).
> > Right click the link and select Open New Container Tab->Work (this is B).
> > 
> > What userContextID does the triggeringPrincipal have?  And is a referrer
> > sent during the load in the B?
> 
> userContextId is 0, I forgot to check the referrer.

This seems like a bug.  Why does the triggeringPrincipal have a userContextId of 0 when neither tab has a userContextId of 0?

I will see if I can get someone to look into this, since Jonathan is on PTO now.

	Carsten Book [:Tomcat]
	Comment 39 • 8 years ago

has problems to apply

adding 1270680 to series file
renamed 1270680 -> Bug-1270680---Part-2-Tests-that-make-sure-image-ca.patch
applying Bug-1270680---Part-2-Tests-that-make-sure-image-ca.patch
patching file browser/components/contextualidentity/test/browser/browser.ini
Hunk #1 FAILED at 9
1 out of 1 hunks FAILED -- saving rejects to file browser/components/contextualidentity/test/browser/browser.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1270680---Part-2-Tests-that-make-sure-image-ca.patch

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 40 • 8 years ago

Rebased.

	Pulsebot
	Comment 41 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d08891438e58
Part 1: Double-key the image cache by origin attribute. r=jdm,seth
https://hg.mozilla.org/integration/mozilla-inbound/rev/20ab9d396613
Part 2: Tests that make sure image cache respect originAttributes. r=jdm,seth
https://hg.mozilla.org/integration/mozilla-inbound/rev/518708a725d5
Part 3 - Get origin attributes from loadInfo in LoadImageWithChannel(). r=tanvi

	Carsten Book [:Tomcat]
	Comment 42 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d08891438e58
https://hg.mozilla.org/mozilla-central/rev/20ab9d396613
https://hg.mozilla.org/mozilla-central/rev/518708a725d5

	Tanvi Vyas[:tanvi] Reporter
	Comment 43 • 8 years ago

This may have caused this regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1279099

	Tanvi Vyas[:tanvi] Reporter
	Comment 44 • 8 years ago

This was backed out in https://bugzilla.mozilla.org/show_bug.cgi?id=1280006

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 45 • 8 years ago

There were many discussions in bug 1279099. Conclusions: We need to check if the API is used in extensions to determine whether we can pass in system principals. I'll do that once dxr finishes indexing addons.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 46 • 8 years ago

Hi, Boris.

In https://bugzilla.mozilla.org/show_bug.cgi?id=1279099#c17, you suggested checking add-ons. But given that add-on indexing is taking forever, is there another way we can resolve this bug without the risk of breaking add-ons?

Thank you.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 47 • 8 years ago

Just found that we have a test index of addons.  https://dxr.mozilla.org/addons/source/

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 48 • 8 years ago

And I found that nsAlertIconListener has gone through lots of changes. The call to LoadImageXPCOM is moved to http://searchfox.org/mozilla-central/source/toolkit/components/alerts/AlertNotification.cpp#311

It uses the mPrincipal of AlertImageRequest, which comes from here: http://searchfox.org/mozilla-central/source/toolkit/components/alerts/AlertNotification.cpp#174

AlertNotification's mPrincipal could be set at these places: http://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla17AlertNotification4InitERK9nsAStringS3_S3_S3_bS3_S3_S3_S3_P12nsIPrincipalb%2C_ZN20nsIAlertNotification4InitERK9nsAStringS2_S2_S2_bS2_S2_S2_S2_P12nsIPrincipalb&redirect=false

In Notification.cpp: http://searchfox.org/mozilla-central/source/dom/notification/Notification.cpp#1834 and DesktopNotification.cpp: http://searchfox.org/mozilla-central/source/dom/notification/Notification.cpp#1834, it seems that the principal won't be null. But the others' principals come from callers of ShowAlertNotification, and many callers didn't pass in principals http://searchfox.org/mozilla-central/search?q=showalertnotification&case=false&regexp=false&path=

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 49 • 8 years ago

There are no direct extension uses of LoadImageXPCOM and LoadImageWithChannelXPCOM. But there are 1882 usages of showAlertNotification() in addons...

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 50 • 8 years ago

I was mistaken. ShowAlertNotification will initialize the alert's principal, but doesn't results in loadImage().

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 51 • 8 years ago

OK it turns out showAlertNotification does result in loadImage, in linux only.

Addons call nsAlertService::ShowAlertNotification() and never pass in the optional argument aPrincipal. In the function an AlertNotification object will be created, with its mPrincipal set as the optional aPrincipal which is almost always null.

I didn't check all 1882 addons, but the urls I saw are all chrome urls.

Boris, do you think we can add an assertion to ensure that this api is only used with chrome urls and just use system principals? Or is there a better way?

	Boris Zbarsky [:bzbarsky]
	Comment 52 • 8 years ago

And assertion is completely useless when addons are involved, yes?

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 53 • 8 years ago

Or maybe throw an NS_ERROR_NOT_AVAILABLE?
http://searchfox.org/mozilla-central/source/toolkit/components/alerts/nsIAlertsService.idl#168

Can we only allow passing chrome urls to showAlertNotification()?

Seth, what do you think?

	Seth Fowler [:seth] [:s2h]
	Comment 54 • 8 years ago

(In reply to Jonathan Hao [:jhao] from comment #53)
> Or maybe throw an NS_ERROR_NOT_AVAILABLE?
> http://searchfox.org/mozilla-central/source/toolkit/components/alerts/
> nsIAlertsService.idl#168
> 
> Can we only allow passing chrome urls to showAlertNotification()?
> 
> Seth, what do you think?

Probably someone who works on the alerts code would know best how to proceed here. The obvious person, to my knowledge, is Kit, but I see that he's on PTO right now. Maybe ask on IRC in #fxteam, or see who else appears in the blame for that code.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 55 • 8 years ago

I saw several reviews related to alerts service by William.
William, do you think we can restrict alert notifications to using chrome image urls?

	Lina Butler [:lina]
	Comment 56 • 8 years ago

Hey Jonathan,

(In reply to Jonathan Hao [:jhao] from comment #51)
> OK it turns out showAlertNotification does result in loadImage, in linux
> only.

OS X, too.

> Addons call nsAlertService::ShowAlertNotification() and never pass in the
> optional argument aPrincipal. In the function an AlertNotification object
> will be created, with its mPrincipal set as the optional aPrincipal which is
> almost always null.

Right. I think there are some chrome JS callers that currently omit the principal, too. I agree it's unfortunate that we can't require the principal without breaking compatibility.

(In reply to Jonathan Hao [:jhao] from comment #53)
> Or maybe throw an NS_ERROR_NOT_AVAILABLE?
> http://searchfox.org/mozilla-central/source/toolkit/components/alerts/
> nsIAlertsService.idl#168
> 
> Can we only allow passing chrome urls to showAlertNotification()?

I'll defer to William, but this seems risky. This would break web notifications...though, as you found in comment 48, they always pass a principal, so maybe we can only require chrome URLs without a principal?

	William Chen [:wchen]
	Comment 57 • 8 years ago

(In reply to Jonathan Hao [:jhao] from comment #55)
> I saw several reviews related to alerts service by William.
> William, do you think we can restrict alert notifications to using chrome
> image urls?

Web notifications use the alerts service so we can't restrict the image to chrome URLs. For callers that don't provide a principal I think it's OK to add the restriction. All such callers probably have system privileges and if they already only use chrome URLs then things shouldn't break.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 58 • 8 years ago

This patch was the same as it was r+ by jdm and seth, except that I removed the change to nsAlertsIconListener.

Bz says nsImageFrame callers can pass a system principal in https://bugzil.la/1279099#c17 so we can keep that change.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 59 • 8 years ago

Hi Chris,

This patch was originally in bug 1279099. You said we should consult bz in https://bugzil.la/1279099#c8, and he said we can pass system principal in nsMenuItemIconX in https://bugzil.la/1279099#c17. Would you please review again? Thank you.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 60 • 8 years ago

Hi William. Would you please review this patch?

	Christoph Kerschbaumer [:ckerschb]
	Comment 61 • 8 years ago

Comment on attachment 8773212 [details] [diff] [review]
Part 4: Pass system principal when calling LoadImage in nsMenuItemIconX.mm.

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

If bz is fine with it then I am fine with it, r=me

::: widget/cocoa/nsMenuItemIconX.mm
@@ +313,5 @@
> +  NS_ENSURE_TRUE(ssm, NS_ERROR_FAILURE);
> +  nsCOMPtr<nsIPrincipal> systemPrincipal;
> +  ssm->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
> +  NS_ENSURE_TRUE(systemPrincipal, NS_ERROR_FAILURE);
> +

I suppose you can just use nsContentUtils::GetSystemPrincipal()

	William Chen [:wchen]
	Comment 62 • 8 years ago

Comment on attachment 8773213 [details] [diff] [review]
Part 5: Use system principal if alert notification got nullptr principal.

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

::: toolkit/components/alerts/AlertNotification.cpp
@@ +312,5 @@
> +  nsCOMPtr<nsIPrincipal> principal = mPrincipal;
> +  if (!principal) {
> +    nsAutoCString scheme;
> +    mURI->GetScheme(scheme);
> +    MOZ_ASSERT(scheme.EqualsLiteral("resource") || scheme.EqualsLiteral("chrome"));

If you're only seeing chrome URIs being used when we don't have a principal, then I think we should just enforce resource or chrome URIs here. Also, you should add a comment to the alerts service IDL [1] to stay that only resource and chrome URIs may be used for the icon image if the principal is not provided.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 63 • 8 years ago

I found that addons can also indirectly use showAlertNotification() by notifications.notify(). According to MDN [1], the url can be remote, so we probably can't restrict that to resource and chrome urls.

Since we don't want to pass system principals when we load remote images, I think the only thing we can do is to still pass nullptr principals. And when we receive nullptr principals in imgLoader::LoadImage(), we use default origin attributes to calculate hash keys. This way, we can also avoid touching nsImageFrame and nsMenuItemIconX callers.

Chris, do you think this is a correct approach?

[1]: https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/notifications

	Christoph Kerschbaumer [:ckerschb]
	Comment 64 • 8 years ago

(In reply to Jonathan Hao [:jhao] from comment #63)
> I found that addons can also indirectly use showAlertNotification() by
> notifications.notify(). According to MDN [1], the url can be remote, so we
> probably can't restrict that to resource and chrome urls.

That's definitely something we want to restrict. The rule of thumb is, never use the systemprincipal where the URL to be loaded is not provided by the system but can be provided by a webpage, addon or whatever.

I looked at the code in a little more detail and probably (I am not entirely sure though, we would have to investigate) we could/should extend nsMenuItemIconX::LoadIcon(nsIURI* aIconURI) with a second argument |nsIPrincipal* aLoadingPrincipal|.

From what I have seen in the code LoadIcon() is called from within SetupIcon() which gets the URI from GetIconURI(). Within GetIconURI() we query the document [1] which I suppose is the loadingDocument. So I suppose we could use the document's principal as the loadingPrincipal for loader->loadImage().

Of course someone who knows the code would have to guide us and tell us how feasible our approach is.

[1] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm#179

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 65 • 8 years ago

This patch is what I said in https://bugzil.la/1270680#c63. Just use default origin attributes when the passed-in principal is nullptr, and not modifying any callers.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #64)
> (In reply to Jonathan Hao [:jhao] from comment #63)
> > I found that addons can also indirectly use showAlertNotification() by
> > notifications.notify(). According to MDN [1], the url can be remote, so we
> > probably can't restrict that to resource and chrome urls.
> 
> That's definitely something we want to restrict. The rule of thumb is, never
> use the systemprincipal where the URL to be loaded is not provided by the
> system but can be provided by a webpage, addon or whatever.

I didn't use any system principal in this patch.

> I looked at the code in a little more detail and probably (I am not entirely
> sure though, we would have to investigate) we could/should extend
> nsMenuItemIconX::LoadIcon(nsIURI* aIconURI) with a second argument
> |nsIPrincipal* aLoadingPrincipal|.
> 
> From what I have seen in the code LoadIcon() is called from within
> SetupIcon() which gets the URI from GetIconURI(). Within GetIconURI() we
> query the document [1] which I suppose is the loadingDocument. So I suppose
> we could use the document's principal as the loadingPrincipal for
> loader->loadImage().

We can file a follow-up bug for this.

> Of course someone who knows the code would have to guide us and tell us how
> feasible our approach is.

Hi Josh, what do you think about this approach?

	Josh Matthews [:jdm]
	Comment 66 • 8 years ago

Comment on attachment 8774286 [details] [diff] [review]
Part 1: Double-key the image cache by origin attribute.

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

This makes sense. Sorry my original suggestion of defaulting to the system principal was unhelpful!

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 67 • 8 years ago

Rebased.

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 68 • 8 years ago

Honza reminded me in another bug that some dummy channels may not have loadInfo, so I changed
NS_ENSURE_TRUE(loadInfo) to if (loadInfo).

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 72 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38093a632b3a

	Jonathan Hao (inactive) [:jhao] Assignee
	Comment 73 • 8 years ago

There are only a couple of lines changed to a testcase Tim wrote so I think he can review.

Tim said I can use just two contexts instead of three to avoid unexpected time-out.

	Tim Huang[:timhuang]
	Comment 74 • 8 years ago

Comment on attachment 8778764 [details] [diff] [review]
Part 3: Turn on todos in browser_forgetaboutsite.js.

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

LGTM, but there are some comments need to be fixed.

::: browser/components/contextualidentity/test/browser/browser_forgetaboutsite.js
@@ +211,5 @@
>      yield BrowserTestUtils.removeTab(tabs[userContextId].tab);
>    }
>  
>    // Check that image cache works with the userContextId.
> +  is(gHits, 2, "The image should be loaded three times.");

"The image should be loaded two times."

@@ +227,5 @@
>                                                        userContextId);
>      yield BrowserTestUtils.removeTab(tabs[userContextId].tab);
>    }
>  
>    // Check that image cache was cleared and the server gets another three hits.

"another two hits"

@@ +233,1 @@
>  }

"The image should be loaded two times."

	Pulsebot
	Comment 77 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d5cbb3e217
Part 1: Double-key the image cache by origin attribute. r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae34bd5397a9
Part 2: Tests that make sure image cache respect originAttributes. r=jdm,seth
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b99d7e48a1
Part 3: Turn on todos in browser_forgetaboutsite.js. r=timhuang

	Carsten Book [:Tomcat]
	Comment 78 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f2d5cbb3e217
https://hg.mozilla.org/mozilla-central/rev/ae34bd5397a9
https://hg.mozilla.org/mozilla-central/rev/19b99d7e48a1