Kamil Jozwiak [:kjozwiak] Reporter
	Description • 8 years ago

The container icons are missing from the File -> New Container Tab under OSX. Looking at the regression range retrieved from mozregression, it looks like bug # 1270680 might have caused the issue. However, looking at recent fixes, bug # 1275432 could have caused this regression as well.

Previously, container images were only missing if a user didn't have a window opened under OSX and went through the file menu (bug # 1272067).

STR:

* open a new install/profile of the latest m-c
* enable containers via about:config -> privacy.userContext.enabled;true
* click on File -> New Container Tab (you'll notice the images are missing)

Regression Range:
=================

18:18.80 INFO: Last good revision: 69c00649c977ab88b19064b4dda04a90f454526f
18:18.80 INFO: First bad revision: 518708a725d51167927dc6f502115c031952a3bb
18:18.80 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=69c00649c977ab88b19064b4dda04a90f454526f&tochange=518708a725d51167927dc6f502115c031952a3bb

Platforms Tested:
=================

* OSX 10.11.5 x64: fx50.0a1 buildID: 20160608030219, changeset: f8ad071a6e14 - Reproducible [Failed]
* Win 10 x64: fx50.0a1 buildID: 20160608030219, changeset: f8ad071a6e14 - Not Reproducible [Passed]
* Ubuntu 14.04.4 x64: fx50.0a1 buildID: 20160608030219, changeset: f8ad071a6e14 - Not Reproducible [Passed]

	Tanvi Vyas[:tanvi]
	Comment 1 • 8 years ago

This is not good for next week.  Jonathan and baku, please take a look at this.  Thanks!

	Tanvi Vyas[:tanvi]
	Comment 2 • 8 years ago

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #1)
> This is not good for next week.  Jonathan and baku, please take a look at
> this.  Thanks!

But https://bugzilla.mozilla.org/show_bug.cgi?id=1279143 and https://bugzilla.mozilla.org/show_bug.cgi?id=1279140 are more important than missing images, so please prioritize accordingly.  Thank you!

	Tanvi Vyas[:tanvi]
	Comment 5 • 8 years ago

Jonathan, are you passing System because Chrome is loading this image?

Did this regression occur because of the image cache bug?

Finally, aren't you on PTO ;)  Not that I don't appreciate the help!

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

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #5)
> Jonathan, are you passing System because Chrome is loading this image?

Actually this change was in Dave's patch in bug 962365, but I somehow lost it when I rebased it for bug 1270680. Dave used system principal so I didn't think much about it, but I do think Chrome is loading the menu icon image, isn't it?

> Did this regression occur because of the image cache bug?

Yes, it is, because with the patch of the image cache bug, we cannot pass null as aLoadingPrincipal to LoadImage().

> Finally, aren't you on PTO ;)  Not that I don't appreciate the help!

My feet needs to rest after walking all around London for days, and I have nothing to do in the airbnb, so...

	Tanvi Vyas[:tanvi]
	Comment 7 • 8 years ago

Comment on attachment 8761820 [details] [diff] [review]
Pass system principal when calling LoadImage in nsMenuItemIconX.mm.

Christoph should review this.

Chris, take a look at https://bugzilla.mozilla.org/attachment.cgi?id=8759032&action=diff#a/image/imgLoader.cpp_sec3.  Jonathan landed that code in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1270680 to error out early if LoadingPrincipal was sent to imgLoader::LoadImage().  There are some images loaded without principals; for example here in nsMenuItemIconX.mm, and potentially other places https://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm#312.

A few things I'm unsure of, that Chris may be aware of:

1) Is this intentionally called with nullptr instead of SystemPrincipal?  If so, why?  Didn't we try to populate all the LoadImage() calls with accurate principals?
https://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm#312

2) Is this only called for Internal Images?  From the name, it sounds like it but can we confirm?

If this is only called for internal images, then using System is probably okay.  Another option is to skip the cache when a principal isn't set.  Its hard to tell which of these options is better.

JKingston makes a good point - why isn't this happening for other chrome images?  Why aren't they going through this code path?

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

Comment on attachment 8761820 [details] [diff] [review]
Pass system principal when calling LoadImage in nsMenuItemIconX.mm.

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

If loader->LoadImage() is called using a nullptr as loadingPrincipal then we end up using the systemPrincipal as the loadingPrincipal for that image load [1]. However, using the systemPrincipal as the loadingPrincipal within loader->LoadImage() bypasses certain security checks which is obviously not desirable. E.g. LoadImage() calls ValidateEntry() which calls ValidateSecurityInfo() which calls ShouldLoadCachedImage() which for example bypasses Mixed content redirect hop checks. I don't know what other side effects this might be causing, but I suppose we need a better fix even for Bug 1270680 and not just replace all the nullptr's for the loadingPrincipal with a  SystemPrincipal.

I suppose it's desirable to consult bz.

[1] http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#785

	Tanvi Vyas[:tanvi]
	Comment 9 • 8 years ago

Looks like this may be a problem here as well: https://bugzilla.mozilla.org/show_bug.cgi?id=1279519

So in this call, using systemPrincipal is probably fine, since these are Chrome images.  But I'm not sure if that is the case in all cases changed in bug 1270680 (where we separated image cache by originattributes); we have to go through those and see if we are using systemPrincipal for loads that are not initiated by chrome but by a webpage.

Boris, Christoph mentioned that while updating the imgLoader code, you mentioned that you wanted to keep certain principals as nullptr instead of systemPrincipal.  Do you have any thoughts on this bug?  When we load chrome images, should we use nullptr, systemPrincipal, or the documents principal when calling imgLoader::LoadImage()?

http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#2044
https://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm#312

With the changes made and landed in Bug 1270680, using nulltpr will cause the image load to fail:
http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#2132
https://bugzilla.mozilla.org/attachment.cgi?id=8759032&action=diff#a/image/imgLoader.cpp_sec3

Instead of requiring the loadingPrincipal, should we just skip the cache if no loadingPrincipal is provided?  If the only time we don't have a loadingPrincipal is for internal images, then it would be fine to not cache those since they are internal anyway.  But there may be other cases as well, which could cause performance issues or more bugs.

	Eiichi
	Comment 10 • 8 years ago

This bug seems to be duplicate of bug 1279430?
Does anyone confirm?

	Eiichi
	Comment 11 • 8 years ago

Bug 1280006 - Backout "Bug 1270680 seems to fix Bug 1279430.

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

[Tracking Requested - why for this release]: Our menus are all broken.

Tanvi, I don't know the answers to your questions offhand.  There have been enough changes to image loading and the security aspects thereof recently that I no longer have a good grasp of how it works, unfortunately.  :(  And in particular, I don't recall which principals I said I wanted to keep nullptr or why without more context....

For the specific case of nsMenuItemX.mm it's probably fine to pass the system principal.  For the general case of nullptr principals, it may be fine to bypass the cache, or to just use a part of the cache that is limited to no-principal loads.  It really depends on which callers are passing in null.

I do admit I don't understand the patches in bug 1270680, which first bail out on nullptr principal, then try to handle it later in the code...

On a somewhat unrelated note, this pattern (in the patches in this bug and in bug 1270680):

  nsCOMPtr<nsIScriptSecurityManager> ssm = nsContentUtils::GetSecurityManager();
  NS_ENSURE_TRUE(ssm, NS_ERROR_FAILURE);
  nsCOMPtr<nsIPrincipal> systemPrincipal;
  ssm->GetSystemPrincipal(getter_AddRefs(systemPrincipal));

seems like a very complicated way of writing:

  nsIPrincipal* systemPrincipal = nsContentUtils::GetSystemPrincipal();

Taking the liberty of adding the tracking flags and regression keyword that should totally be here so we don't accidentally ship this....

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

> For the general case of nullptr principals, it may be fine to bypass the cache,
> or to just use a part of the cache that is limited to no-principal loads.

No, that's just wrong.  It's not fine to bypass the cache at all.

> then it would be fine to not cache those since they are internal anyway.

No, because what's cached is the _decoded_ image in many cases.  So if you bypass the cache you will redo all the work of reading the image from disk, sanity-checking it, decoding it, etc.  So bypassing the cache is not OK.  This means we need to either make this codepath work with nullptr principals (e.g. allow cache entries and cache keys without origin attributes) or change the API to not allow nullptr principals at all and update all callers.  Which one we do, again, depends on which current callers pass nullptr, how we currently handle nullptr, and what behavior we want in those cases.

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

Since we can't bypass the cache and we also don't want to use system principals everywhere. I think we should use default origin attributes to create cache keys when LoadImage() got nullptr principals. The cache behavior will be the same as changing nullptr to system principals, but the security checks won't be bypassed.

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

The callers that pass nullptr principals to LoadImage are:
1) https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp?from=nsImageFrame.cpp#2216
2) https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm#314
3) https://dxr.mozilla.org/mozilla-central/source/toolkit/system/gnome/nsAlertsIconListener.cpp#257

and a couple more in tests.

Boris, if you don't have enough context to decide whether we can use system principals in these callers, do you know someone who has?

Those all seem to be icon loads, so probably all internal images without internet connections, but I'm not sure about that.

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

The nsImageFrame and nsMenuItemIconX callers could certainly pass a system principal: their URIs are very obvious and all internal-ish.

For the nsAlertsIconListener.cpp case, there is no guarantee of anything at all about the URI: it's an XPCOM API that anyone (well, browser code or extensions) can call, passing in any URI.  I would check whether extensions actually use it, but of course mxr is down for addons.

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

Also, isn't the image-loading API public too?  Have you checked for direct extension uses of it?  (Not that you can, right now, with mxr down.)

	Marcia Knous [:marcia]
	Comment 19 • 8 years ago

Tracking 50+ for this regression due to missing UI.

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

Actually this regression is already resolved after we landed the backout patches in bug 1280006.
However, there are many important discussions above, so I'm not sure whether we should close this bug or not.

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

(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #17)
> The nsImageFrame and nsMenuItemIconX callers could certainly pass a system
> principal: their URIs are very obvious and all internal-ish.
> 
> For the nsAlertsIconListener.cpp case, there is no guarantee of anything at
> all about the URI: it's an XPCOM API that anyone (well, browser code or
> extensions) can call, passing in any URI.  I would check whether extensions
> actually use it, but of course mxr is down for addons.

(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #18)
> Also, isn't the image-loading API public too?  Have you checked for direct
> extension uses of it?  (Not that you can, right now, with mxr down.)

Looks like mxr is gone for good. Remaining mxr trees such as addons will be indexed into dxr, tracked in bug 1281443.

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

I'm going to close this bug as the regression was already resolved when we backed out image caching double-key. I'll copy the important discussions to bug 1270680.

	Tanvi Vyas[:tanvi]
	Comment 23 • 8 years ago

Kamil, can you do a quick confirmation?

	Kamil Jozwiak [:kjozwiak] Reporter
	Comment 24 • 8 years ago

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #23)
> Kamil, can you do a quick confirmation?

Quickly confirmed that all the images are correctly appearing using the following platforms:

* Ubuntu 14.04.4 LTS [VM]
** using fx50.0a1 buildid: 20160705030222 changeset: c9a70b64f2fa

* Win 10 x64 (Version: 1511 OS Build: 10586.318) [VM]
** using fx50.0a1 buildid: 20160705030222 changeset: c9a70b64f2fa

* Win 7 x64 (Version: 6.1 Build: 7601 SP1) [VM]
** using fx50.0a1 buildid: 20160705030222 changeset: c9a70b64f2fa

* OSX 10.9.5 x64 [VM]
** using fx50.0a1 buildid: 20160705030222 changeset: c9a70b64f2fa

* OSX 10.11.5 x64 [VM]
** using fx50.0a1 buildid: 20160705030222 changeset: c9a70b64f2fa

	Sylvestre Ledru [:Sylvestre]
	Comment 25 • 8 years ago

Not tracking anymore as it is a dup and the dup has been fixed