image cache should respect originAttributes

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: tanvi, Assigned: jhao)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [OA][userContextId][domsecurity-active][tor])

Attachments

(3 attachments, 23 obsolete attachments)

10.55 KB, patch
jhao
: review+
Details | Diff | Splinter Review
3.22 KB, patch
jhao
: review+
Details | Diff | Splinter Review
4.18 KB, patch
jhao
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 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 +++
(Reporter)

Updated

3 years ago
Component: ImageLib → DOM: Security
Whiteboard: [OA][userContextId] → [OA][userContextId][domsecurity-backlog]
(Assignee)

Updated

3 years ago
Assignee: nobody → jhao

Updated

3 years ago
Priority: -- → P1
(Assignee)

Comment 1

3 years ago
Created attachment 8750697 [details] [diff] [review]
image isolation with origin attributes (from 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?
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 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.
(Assignee)

Comment 4

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

Updated

3 years ago
Blocks: 1238183
(Assignee)

Comment 5

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

Comment 6

2 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.
Flags: needinfo?(josh)
(Assignee)

Updated

2 years ago
Iteration: --- → 49.3 - Jun 6
(Assignee)

Comment 7

2 years ago
Created attachment 8754730 [details] [diff] [review]
Double-key the image cache by origin attribute

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.
Attachment #8754730 - Flags: review?(seth)
(Assignee)

Updated

2 years ago
Attachment #8750697 - Attachment is obsolete: true
Flags: needinfo?(josh)
(Assignee)

Comment 8

2 years ago
Created attachment 8755359 [details] [diff] [review]
Make sure image cache respect originAttributes

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.
Attachment #8755359 - Flags: review?(seth)
(Assignee)

Comment 9

2 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?
Attachment #8754730 - Flags: review?(josh)
(Assignee)

Updated

2 years ago
Attachment #8755359 - Flags: review?(josh)

Updated

2 years ago
Attachment #8755359 - Flags: review?(josh) → review+
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...
Attachment #8754730 - Flags: review?(josh)
(Reporter)

Updated

2 years ago
Blocks: 1276412
(Assignee)

Comment 11

2 years ago
Created attachment 8757855 [details] [diff] [review]
Double-key the image cache by origin attributes.

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.
Attachment #8754730 - Attachment is obsolete: true
Attachment #8754730 - Flags: review?(seth)
Attachment #8757855 - Flags: review?(seth)
Attachment #8757855 - Flags: review?(josh)
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.
Attachment #8757855 - Flags: review?(josh) → review+
(Assignee)

Comment 13

2 years ago
Created attachment 8758104 [details] [diff] [review]
Double-key the image cache by origin attribute.
Attachment #8758104 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8757855 - Attachment is obsolete: true
Attachment #8757855 - Flags: review?(seth)
(Assignee)

Updated

2 years ago
Attachment #8758104 - Flags: review?(seth)
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.
Attachment #8755359 - Flags: review?(seth) → review+
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.)
Attachment #8758104 - Flags: review?(seth) → review+
(Assignee)

Comment 16

2 years ago
Created attachment 8758536 [details] [diff] [review]
Part 1: Double-key the image cache by origin attribute.

Thanks so much to Josh and Seth.
Attachment #8758536 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8758104 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Created attachment 8758537 [details] [diff] [review]
Part 2: Tests that make sure image cache respect originAttributes.
Attachment #8758537 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8755359 - Attachment is obsolete: true
(Assignee)

Comment 18

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

Comment 19

2 years ago
Created attachment 8758798 [details] [diff] [review]
Part 1: Double-key the image cache by origin attribute.

MozReview-Commit-ID: AKmMZe0oNT2
Attachment #8758798 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8758536 - Attachment is obsolete: true
(Assignee)

Comment 20

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

Updated

2 years ago
Attachment #8758798 - Attachment is obsolete: true
(Assignee)

Comment 21

2 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.
Attachment #8758536 - Attachment is obsolete: false
(Assignee)

Comment 22

2 years ago
Created attachment 8759032 [details] [diff] [review]
Part 1: Double-key the image cache by origin attribute.

Fix an error in nsAlertsIconListener I made when rebasing.
Attachment #8759032 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8758536 - Attachment is obsolete: true
(Assignee)

Comment 23

2 years ago
Created attachment 8759038 [details] [diff] [review]
Part 3 - Use triggering principal when loading principal is null.

Loading principal could be null if this is a top-level load, so we use triggering principal instead.
Attachment #8759038 - Flags: review?(ckerschb)
(Assignee)

Comment 24

2 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
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?
Attachment #8759038 - Flags: review?(ckerschb)
(Assignee)

Comment 26

2 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.
(Assignee)

Comment 27

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

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.
Attachment #8759135 - Flags: review?(ckerschb)
(Assignee)

Updated

2 years ago
Attachment #8759038 - Attachment is obsolete: true
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.
Attachment #8759135 - Flags: review?(ckerschb) → feedback+
(Assignee)

Comment 29

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

Thank you, Christoph.

Tanvi, please take a look. Thanks a lot.
Attachment #8759151 - Flags: review?(tanvi)
(Assignee)

Updated

2 years ago
Attachment #8759135 - Attachment is obsolete: true
(Reporter)

Comment 31

2 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!
(Reporter)

Comment 32

2 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.
Attachment #8759151 - Flags: review?(tanvi) → review+
(Assignee)

Comment 33

2 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.
(Assignee)

Comment 34

2 years ago
Created attachment 8759494 [details] [diff] [review]
Part 3 - Get origin attributes from loadInfo when the load is a top-level document.

Thanks, Tanvi. I fixed the nits.
Attachment #8759494 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8759151 - Attachment is obsolete: true
(Assignee)

Comment 35

2 years ago
Created attachment 8759495 [details] [diff] [review]
Part 3 - Get origin attributes from loadInfo in LoadImageWithChannel().

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?
Attachment #8759495 - Flags: review?(tanvi)
Attachment #8759495 - Flags: feedback?(tanvi)
(Assignee)

Updated

2 years ago
Attachment #8759495 - Flags: feedback?(tanvi)
(Reporter)

Comment 36

2 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?
(Reporter)

Updated

2 years ago
Attachment #8759495 - Flags: review?(tanvi) → review+
(Assignee)

Comment 37

2 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.
(Reporter)

Comment 38

2 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.
(Reporter)

Updated

2 years ago
Attachment #8759494 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
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
Flags: needinfo?(jhao)
Keywords: checkin-needed
Whiteboard: [OA][userContextId][domsecurity-backlog] → [OA][userContextId][domsecurity-active]
(Assignee)

Comment 40

2 years ago
Created attachment 8760212 [details] [diff] [review]
Part 2: Tests that make sure image cache respect originAttributes.

Rebased.
Attachment #8760212 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8758537 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Flags: needinfo?(jhao)
Keywords: checkin-needed

Comment 41

2 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
Keywords: checkin-needed

Comment 42

2 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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Reporter)

Comment 43

2 years ago
This may have caused this regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1279099
Depends on: 1279519
(Reporter)

Comment 44

2 years ago
This was backed out in https://bugzilla.mozilla.org/show_bug.cgi?id=1280006
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 45

2 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.
See Also: → bug 1279099
(Reporter)

Updated

2 years ago
Depends on: 1281443
Depends on: 1280948
No longer depends on: 1281443
status-firefox50: fixed → ---
Target Milestone: mozilla50 → ---
(Assignee)

Comment 46

2 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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 47

2 years ago
Just found that we have a test index of addons.  https://dxr.mozilla.org/addons/source/
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 49

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

Comment 50

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

Comment 51

2 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?
Flags: needinfo?(bzbarsky)
And assertion is completely useless when addons are involved, yes?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 53

2 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?
Flags: needinfo?(seth)
(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.
Flags: needinfo?(seth) → needinfo?(kcambridge)
(Assignee)

Comment 55

2 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?
Flags: needinfo?(wchen)
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?
Flags: needinfo?(kcambridge)
(Assignee)

Updated

2 years ago
Blocks: 962365

Updated

2 years ago
Whiteboard: [OA][userContextId][domsecurity-active] → [OA][userContextId][domsecurity-active][tor]

Updated

2 years ago
Whiteboard: [OA][userContextId][domsecurity-active][tor] → [OA][userContextId][domsecurity-active]
(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.
Flags: needinfo?(wchen)
(Assignee)

Comment 58

2 years ago
Created attachment 8773209 [details] [diff] [review]
Part 1: Double-key the image cache by origin attribute.

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.
Attachment #8773209 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8759032 - Attachment is obsolete: true
(Assignee)

Comment 59

2 years ago
Created attachment 8773212 [details] [diff] [review]
Part 4: Pass system principal when calling LoadImage in nsMenuItemIconX.mm.

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.
Attachment #8773212 - Flags: review?(ckerschb)
(Assignee)

Comment 60

2 years ago
Created attachment 8773213 [details] [diff] [review]
Part 5: Use system principal if alert notification got nullptr principal.

Hi William. Would you please review this patch?
Attachment #8773213 - Flags: review?(wchen)
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()
Attachment #8773212 - Flags: review?(ckerschb) → review+
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.
Attachment #8773213 - Flags: review?(wchen) → review+
(Assignee)

Comment 63

2 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
Flags: needinfo?(ckerschb)
(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
Flags: needinfo?(ckerschb)
(Assignee)

Comment 65

2 years ago
Created attachment 8774286 [details] [diff] [review]
Part 1: Double-key the image cache by origin attribute.

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?
Attachment #8774286 - Flags: review?(josh)
(Assignee)

Updated

2 years ago
Attachment #8759495 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8773209 - Attachment is obsolete: true
Attachment #8773212 - Attachment is obsolete: true
Attachment #8773213 - Attachment is obsolete: true
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!
Attachment #8774286 - Flags: review?(josh) → review+
(Assignee)

Comment 67

2 years ago
Created attachment 8778069 [details] [diff] [review]
Part 2: Tests that make sure image cache respect originAttributes.

Rebased.
Attachment #8778069 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8760212 - Attachment is obsolete: true
(Assignee)

Comment 68

2 years ago
Created attachment 8778070 [details] [diff] [review]
Part 1: Double-key the image cache by origin attribute.

Honza reminded me in another bug that some dummy channels may not have loadInfo, so I changed
NS_ENSURE_TRUE(loadInfo) to if (loadInfo).
Attachment #8778070 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8774286 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8778069 - Attachment is obsolete: true
(Assignee)

Comment 71

2 years ago
Created attachment 8778122 [details] [diff] [review]
Part 2: Tests that make sure image cache respect originAttributes.
Attachment #8778122 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8778120 - Attachment is obsolete: true
(Assignee)

Comment 73

2 years ago
Created attachment 8778764 [details] [diff] [review]
Part 3: Turn on todos in browser_forgetaboutsite.js.

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.
Attachment #8778764 - Flags: review?(tihuang)
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."
Attachment #8778764 - Flags: review?(tihuang) → review+
(Assignee)

Comment 75

2 years ago
Created attachment 8778769 [details] [diff] [review]
Part 3: Turn on todos in browser_forgetaboutsite.js.
Attachment #8778769 - Flags: review?(tihuang)
(Assignee)

Updated

2 years ago
Attachment #8778764 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8778769 - Flags: review?(tihuang) → review+
(Assignee)

Comment 76

2 years ago
Created attachment 8778778 [details] [diff] [review]
Part 3: Turn on todos in browser_forgetaboutsite.js.
Attachment #8778778 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8778769 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 77

2 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
Keywords: checkin-needed

Comment 78

2 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
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Duplicate of this bug: 962365
Whiteboard: [OA][userContextId][domsecurity-active] → [OA][userContextId][domsecurity-active][tor]
You need to log in before you can comment on or make changes to this bug.