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 +++

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

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?

(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.

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.

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

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.

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.

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.

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?

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...

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.

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.

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.

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.)

Thanks so much to Josh and Seth.

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

MozReview-Commit-ID: AKmMZe0oNT2

(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

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.

Fix an error in nsAlertsIconListener I made when rebasing.

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

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

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?

(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.

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.

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.

Thank you, Christoph. Tanvi, please take a look. Thanks a lot.

(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!

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.

(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.

Thanks, Tanvi. I fixed the nits.

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?

(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?

(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.

(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.

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

Rebased.

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

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

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

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

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.

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.

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

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=

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

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

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?

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

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?

(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.

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

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?

(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.

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.

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.

Hi William. Would you please review this patch?

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()

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.

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

(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

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?

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!

Rebased.

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

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.

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."

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

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