Boris, Seth, within this patch we would like to make the imageLoader use AsyncOpen2(), which means that we will perform security checks within AsyncOpen2() and hence can (basically) remove nsContentUtils::CanLoadImage(). Since the imageLoader is rather complicated I didn't remove all of the code, but rather commented some of it out and also left some explanations why I think we can remove nsContentUtils::CanLoadImage. After the first round of reviews I will clean up the code and prepare it for a final review, but I think it makes sense to be caucious when changing the imageLoader. As part of this effort we now have to pass the contentPolicyType around, e.g. so ShouldLoadCachedImage() has the right type information available for checks. The other thing is, that we have to pass the "corsMode" so we have it available when we call NewChannel. Also I updated the NewChannel creation with this line |if (requestingNode && aLoadingPrincipal) {|, which seems more accurate and does also match the css loader behavior where we did a similar thing [1,2]. Thanks for your help! [1] http://hg.mozilla.org/mozilla-central/diff/9535c05d5022/layout/style/Loader.cpp#l1.58 [2] http://hg.mozilla.org/mozilla-central/diff/9535c05d5022/layout/style/Loader.cpp#l1.97

Please see comment 3.

Boris, within this patch I cover all the tests that need to be updated, one of them needs special attention: > browser/base/content/test/general/browser_aboutHome.js where I am not sure how to proceed, I left a note in the patch explaining why the test fails. I suppose we don't to use |contentaccessible=yes| to fix the problem, right? The two failing web-platform tests [1] need some further investigation. At the moment I can't reproduce locally, but I'll have a look at those two ASAP. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd054f774d19&selectedJob=17677508

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7) > The two failing web-platform tests [1] need some further investigation. At > the moment I can't reproduce locally, but I'll have a look at those two ASAP. This changeset [1] definitely needs investigation in order to fix service-worker/fetch-request-css-images.https.html. [1] http://hg.mozilla.org/mozilla-central/rev/74ab992b6952

Comment on attachment 8727232 [details] [diff] [review] bug_1206961_imgloader_asyncopen2_image_changes.patch I expect Seth can just review this.

Comment on attachment 8727233 [details] [diff] [review] bug_1206961_asyncopen_imgloader_dom_changes.patch Will the right SetBlockedRequest calls still happen? If so, what will cause them to happen? >+ SetBlockedRequest(aNewURI, nsIContentPolicy::REJECT_REQUEST); This seems clearly wrong if the actual rejection was with one of the other REJECT values, yes? And yes, the actual value matters very much; we treat them differently in layout last I checked. In general, I'm not quite sure what you're looking for out of this review. If you're looking for checks on whether your assessments of whether the various CanLoadImage calls are needed, that would be _much_ simpler in the form of a comment listing what the relevant callsites are and how you're assessing them. If you're looking for an actual "this is OK to check in" review, then this is not ok to check in, obviously.

Comment on attachment 8727234 [details] [diff] [review] bug_1206961_asyncopen_imgloader_layout_changes.patch This is OK in the sense that if nsContentUtils::LoadImage ends up doing all the relevant checks itself then CanLoadImage is not needed at these callsites. That said, afaict the checks it will do will use the "wrong" (or at least different) context node in the two cases that passed mContent to CanLoadImage, no?

Comment on attachment 8727236 [details] [diff] [review] bug_1206961_asyncopen_imgloader_test_updates.patch >+++ b/browser/base/content/test/general/browser_aboutHome.js I don't understand why this new test is being added, what it's trying to test, and why it's expected to pass, so I don't know how to answer the question in comment 7.... >+++ b/dom/base/test/test_bug1118689.html This change shows what I expect is a bug in the new setup. In particular, what happens if the data document created via createHTMLDocument has an <img> linking to a URL that's already in the image cache. Now that we're doing the content policy checks at AsyncOpen time, do we even do them, or just end up using the cached image? Quite apart from the correctness issue, there's a performance issue here too: we want to do as little work as possible for <img> in data documents; we don't want to spend a bunch of time dealing with it only to discover that hey, we're in a data document and it was all pointless. >+++ b/dom/security/test/csp/test_docwrite_meta.html Why the change to this test? Is this the big about using the wrong rejection reason in nsImageLoadingContent causing us to have an incorrect layout here? >+++ b/dom/xml/crashtests/crashtests.list >+++ b/layout/reftests/svg/image/reftest.list >+++ b/parser/htmlparser/tests/mochitest/test_bug102699.html These are all showing the same problem with data documents I talk about above, yes? But anyway, if we do change the invariant about when LoadImage is called, the right solution is to remove the now-bogus assertion, not to pretend like it's OK to assert something now clearly false and annotate a bunch of tests. I can't tell you anything useful about the two wpt failures, sorry.

Comment on attachment 8727232 [details] [diff] [review] bug_1206961_imgloader_asyncopen2_image_changes.patch Review of attachment 8727232 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand these changes well enough to review them. Could you answer my "what are the implications..." questions below and then re-request review? ::: image/imgLoader.cpp @@ +753,5 @@ > + aCORSMode == imgIRequest::CORS_NONE > + ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS > + : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS; > + if (aCORSMode == imgIRequest::CORS_ANONYMOUS) { > + securityFlags |= nsILoadInfo::SEC_COOKIES_SAME_ORIGIN; I'm not sure that this is clearer than just having a series of |if| branches that set the right security flags directly for each possible imgIRequest::CORS_* value. @@ +768,4 @@ > rv = NS_NewChannelWithTriggeringPrincipal(aResult, > aURI, > requestingNode, > + aLoadingPrincipal, What are the implications of this change? I'm out of my depth here, I'm afraid. @@ +801,5 @@ > + if (aLoadingPrincipal) { > + inherit = nsContentUtils:: > + ChannelShouldInheritPrincipal(aLoadingPrincipal, > + aURI, > + false, // aInheritForAboutBlank Normally in ImageLib we write this as |/* aInheritForAboutBlank = */ false|. @@ +804,5 @@ > + aURI, > + false, // aInheritForAboutBlank > + false); // aForceInherit > + } > + *aForcePrincipalCheckForCacheEntry = inherit; Do we use |inherit| elsewhere? Why not just set |*aForcePrincipalCheckForCachEntry| in either an |if| or an |else| branch? @@ -2335,5 @@ > // it says that the entry isn't valid any more, we'll only use the entry > // we're getting if the channel is loading from the cache anyways. > - // > - // XXX -- should this be changed? it's pretty much verbatim from the old > - // code, but seems nonsensical. Are you sure you should have removed this comment? I don't think this relates to what you changed. @@ +2311,5 @@ > + nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo(); > + // if there is a loadInfo, use the right contentType, otherwise > + // default to the internal image type > + nsContentPolicyType policyType = loadInfo ? > + loadInfo->InternalContentPolicyType() : Align '?' and ':' with '=' please. @@ +2318,3 @@ > if (ValidateEntry(entry, uri, nullptr, nullptr, RP_Default, > nullptr, aObserver, aCX, requestFlags, > + policyType, false, nullptr, We used to always use |TYPE_INVALID| for |policyType| here; what are the implications of this change?

(In reply to Seth Fowler [:seth] [:s2h] from comment #13) > I don't understand these changes well enough to review them. Could you > answer my "what are the implications..." questions below and then re-request > review? Sure thing, let me try to explain. > ::: image/imgLoader.cpp > @@ +753,5 @@ > > + aCORSMode == imgIRequest::CORS_NONE > > + ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS > > + : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS; > > + if (aCORSMode == imgIRequest::CORS_ANONYMOUS) { > > + securityFlags |= nsILoadInfo::SEC_COOKIES_SAME_ORIGIN; > > I'm not sure that this is clearer than just having a series of |if| branches > that set the right security flags directly for each possible > imgIRequest::CORS_* value. The reason I aligned it this way is because reviewers of other patches (e.g. see scriptLoader) prefered this formating style. Since we already use this format within our codebase (basically everywhere) where we need to pass CORS flags to NS_NewChannel I would like to keep it that way. > @@ +768,4 @@ > > rv = NS_NewChannelWithTriggeringPrincipal(aResult, > > aURI, > > requestingNode, > > + aLoadingPrincipal, > > What are the implications of this change? I'm out of my depth here, I'm > afraid. In fact it shouldn't make a difference. Because if we have a node we should also have a loadingPrincipal. It's just clearer to write it this way, to be really explicit that we use the systemPrincipal in the *else* branch. If you look closely at the old code, it should do the same thing. We assign the systemPrincipal to *triggeringPrincipal* in case aLoadingPrincipal is null. > @@ +801,5 @@ > > + false, // aInheritForAboutBlank > > Normally in ImageLib we write this as |/* aInheritForAboutBlank = */ false|. Updated. > @@ +804,5 @@ > > + aURI, > > + false, // aInheritForAboutBlank > > + false); // aForceInherit > > + } > > + *aForcePrincipalCheckForCacheEntry = inherit; > > Do we use |inherit| elsewhere? Why not just set > |*aForcePrincipalCheckForCachEntry| in either an |if| or an |else| branch? You are right, not used elsewhere - updated that as well! > @@ -2335,5 @@ > > // it says that the entry isn't valid any more, we'll only use the entry > > // we're getting if the channel is loading from the cache anyways. > > - // > > - // XXX -- should this be changed? it's pretty much verbatim from the old > > - // code, but seems nonsensical. > > Are you sure you should have removed this comment? I don't think this > relates to what you changed. > @@ +2311,5 @@ > > + nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo(); > > + // if there is a loadInfo, use the right contentType, otherwise > > + // default to the internal image type > > + nsContentPolicyType policyType = loadInfo ? > > + loadInfo->InternalContentPolicyType() : > > Align '?' and ':' with '=' please. > > @@ +2318,3 @@ > > if (ValidateEntry(entry, uri, nullptr, nullptr, RP_Default, > > nullptr, aObserver, aCX, requestFlags, > > + policyType, false, nullptr, > > We used to always use |TYPE_INVALID| for |policyType| here; what are the > implications of this change? ValidateEntry calls ValidateSecurityInfo which calls ShouldLoadCachedImage which calls NS_CheckContentLoadPolicy which internally calls all nsIContentPolicies. For example, MixedContentBlocker asserts that the passed contentpolicyType is valid (in other words, not TYPE_INVALID). I think what happenend is that one of the nsContentUtils::CanLoadImage() checks already blocked the load before we got to this point. Now that we are removing nsContentUtils::CanLoadImage() we hit this codepath, hence we should pass the right contentPolicyType.

(In reply to Boris Zbarsky [:bz] from comment #10) > Comment on attachment 8727233 [details] [diff] [review] > bug_1206961_asyncopen_imgloader_dom_changes.patch > > Will the right SetBlockedRequest calls still happen? If so, what will cause > them to happen? > > >+ SetBlockedRequest(aNewURI, nsIContentPolicy::REJECT_REQUEST); I am not so sure that this is actually wrong, because nsContentUtils::CanLoadImage() returns REJECT_REQUEST if the loading is blocked [1]. Or are you saying that if nsContentUtils::LoadImage() returns an error (not security related) we incorreclty call SetBlockedRequest()? I am not sure what implications we are facing when calling SetBlockedRequest() for an error that is not security related. Other than that I suppose the change would be correct. [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#2973 > This seems clearly wrong if the actual rejection was with one of the other > REJECT values, yes? And yes, the actual value matters very much; we treat > them differently in layout last I checked. As mentioned above, does other REJECT values include ERROR_* values or just REJECTs? > In general, I'm not quite sure what you're looking for out of this review. > If you're looking for checks on whether your assessments of whether the > various CanLoadImage calls are needed, that would be _much_ simpler in the > form of a comment listing what the relevant callsites are and how you're > assessing them. If you're looking for an actual "this is OK to check in" > review, then this is not ok to check in, obviously. My goal was to make an assessment of what CanLoadImage() calls we can remove. I cleaned up the code and now I am really asking for a review :-) Btw, you asked about cached images when reviewing a different patch. We still do perform a security check on cached images. ShouldLoadCachedImage() kicks in correctly. A bunch of the mixedContentblocker and also CSP tests make sure that we perform the right contentpolicyChecks on cached images. One other question is, CanLoadImage also performs an assessment for the appType, do we have to take that into account for the new security checks within asyncOpen2() as well? [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#2988

(In reply to Boris Zbarsky [:bz] from comment #11) > Comment on attachment 8727234 [details] [diff] [review] > bug_1206961_asyncopen_imgloader_layout_changes.patch > That said, afaict the checks it will do will use the "wrong" (or at least > different) context node in the two cases that passed mContent to > CanLoadImage, no? That is true, we end up using a different context for securityChecks within asyncOpen2(). How much of an assessment do we need to perform to make sure we do the right thing. Thanks for you help Boris! Wants we agreed on a patch I'll make those few failing tests work and look into the assertion once again! The failing tests are unrelated to the changes we are perfomring within those patches (besides the assertion :-) ).

> because nsContentUtils::CanLoadImage() returns REJECT_REQUEST if the loading is blocked [1] No, it returns the actual nsIContentPolicy::* decision, if the loading is blocked by content policy. Then nsImageLoadingContent::UpdateImageState treates REJECT_SERVER/REJECT_REQUEST/REJECT_TYPE differently in terms of what it means about the state of the image, and hence about what CSS rules match, what nsIFrames get constructed, and so forth. > As mentioned above, does other REJECT values include ERROR_* values or just REJECTs? I'm not sure what you mean.... The important part for images is that if an image load is blocked by content policy then the nsImageLoadingContent needs to know the exact return value of nsIContentPolicy::shouldLoad that caused it to be blocked. If we were blocked for some other reason, not content policy, we want to treat it as REJECT_REQUEST. > do we have to take that into account for the new security checks within asyncOpen2() as well? I believe you do, yes. Otherwise you will break inserting file:// images (the normal thing to do) into an HTML mail being composed in Thunderbird. > How much of an assessment do we need to perform to make sure we do the right thing. Well, primarily this will affect things like adblock plus, which actually want to get to the DOM node from the content policy stuff, yes?

Comment on attachment 8728675 [details] [diff] [review] bug_1206961_asyncopen_imgloader_dom_changes.patch The nsImageLoadingContent bits are definitely wrong; see comment 17... Why is the nsDocument::MaybePreLoadImage change OK? I don't think it is... in particular, I don't see where you end up using TYPE_INTERNAL_IMAGE_PRELOAD for the content policy check.

(In reply to Boris Zbarsky [:bz] from comment #17) > If we were blocked for some other reason, not content > policy, we want to treat it as REJECT_REQUEST. Ok, now I see that, thanks. So here we definitely need to find a way to pass the "deciscion" back up from asyncOpen2(). I don't think that's going to be easy, so maybe we should just keep that check there. > > do we have to take that into account for the new security checks within asyncOpen2() as well? > > I believe you do, yes. Otherwise you will break inserting file:// images > (the normal thing to do) into an HTML mail being composed in Thunderbird. Ok, so that also needs investigation and needs to be fixed within this patch - thanks for the info!

(In reply to Boris Zbarsky [:bz] from comment #18) > Comment on attachment 8728675 [details] [diff] [review] > bug_1206961_asyncopen_imgloader_dom_changes.patch > > The nsImageLoadingContent bits are definitely wrong; see comment 17... > > Why is the nsDocument::MaybePreLoadImage change OK? I don't think it is... > in particular, I don't see where you end up using > TYPE_INTERNAL_IMAGE_PRELOAD for the content policy check. You are right here as well - we should keep that check.

Jonas, if you look at the changes of nsImageLoadingContent.cpp within [1] you will see that we changed the behavior of SetBlockedRequest, which used to use the actual decision of the contentPolicy (cpDecison) as the third argument. Now that we are going to remove nsContentUtils::CanLoadImage() we don't have access to that information anymore since asyncOpen2() only returns NS_ERROR_CONTENT_BLOCKED if a contentPolicy check fails within Asycnopen2(). I don't think there is any easy fix for that problem. So we have two options: a) We create new NS_ERROR_* failure cases which map to the actual decision, or b) we keep the security check the way it was with the penalty of performing security checks twice. Any suggestions? [1] https://bugzilla.mozilla.org/attachment.cgi?id=8728675&action=diff

We should talk to someone who knows this code well and can describe what behavior they are trying to accomplish. We always have the option of returning more descriptive error codes for example. But rather than guess at what the requirements are here, lets set up a meeting.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #21) > Jonas, if you look at the changes of nsImageLoadingContent.cpp within [1] > you will see that we changed the behavior of SetBlockedRequest, which used > to use the actual decision of the contentPolicy (cpDecison) as the third > argument. Now that we are going to remove nsContentUtils::CanLoadImage() we > don't have access to that information anymore since asyncOpen2() only > returns NS_ERROR_CONTENT_BLOCKED if a contentPolicy check fails within > Asycnopen2(). I don't think there is any easy fix for that problem. So we > have two options: > a) We create new NS_ERROR_* failure cases which map to the actual decision, > or > b) we keep the security check the way it was with the penalty of performing > security checks twice. > > Any suggestions? > > [1] https://bugzilla.mozilla.org/attachment.cgi?id=8728675&action=diff Boris, what do you think? Should we add new NS_ERROR_* codes and propagate them up the stack from the nsContentSecurityManager so we can remap the actual policy decision within nsImageLoadingContent? Or is it more complicated than that and we should set up a meeting?

> so maybe we should just keep that check there. Note that doing two content policy checks per image load may also not be great, both in terms of performance and in terms of behavior of things like existing ad blocker extensions. > You are right here as well - we should keep that check. But not do a separate check with a different content type later on... > Should we add new NS_ERROR_* codes and propagate them up the stack from the nsContentSecurityManager so we > can remap the actual policy decision within nsImageLoadingContent? I suspect that would be fine, yes, as long as (1) no one is currently depending on the exact error codes from that stuff and (2) those error codes are not being propagated out to script anywhere (and thus possibly leaking information about blocking state of stuff to web page scripts). Maybe we can simply remap them all to the same thing in xpconnect or something to avoid having to audit a bunch of codepaths. Another slightly more fragile option would be that the blocked cases (maybe only the ones that aren't REJECT_REQUEST?) would QI the context node to nsIImageLoadingContent and tell it why the blocking is happening. I say more fragile, because it would involve action at a distance whereby the LoadImage call indirectly changes some nsImageLoadingContent members. Anyway, I'm happy to meet and talk about this too. Now that I've paged this stuff back in, I probably satisfy the "someone who knows this code well" requirement...

(In reply to Boris Zbarsky [:bz] from comment #24) > Note that doing two content policy checks per image load may also not be > great, I totally agree, we should definitely avoid that scenario of doing content checks twice. > > Should we add new NS_ERROR_* codes and propagate them up the stack from the nsContentSecurityManager so we > > can remap the actual policy decision within nsImageLoadingContent? > > I suspect that would be fine, yes, as long as (1) no one is currently > depending on the exact error codes from that stuff and (2) those error codes > are not being propagated out to script anywhere (and thus possibly leaking > information about blocking state of stuff to web page scripts). Maybe we > can simply remap them all to the same thing in xpconnect or something to > avoid having to audit a bunch of codepaths. Not sure if we should, but theoretically we could only propagate the new errors up the callstack if the content type is TYPE_*_IMAGE. > Another slightly more fragile option would be that the blocked cases (maybe > only the ones that aren't REJECT_REQUEST?) would QI the context node to > nsIImageLoadingContent and tell it why the blocking is happening. I say > more fragile, because it would involve action at a distance whereby the > LoadImage call indirectly changes some nsImageLoadingContent members. I thought about that as well, but in contract to ::CanLoadImage, where we pass the this pointer as the context (namely the nsIImageLoadingContent), we pass |aDocument| to ::LoadImage(). If we could somehow query the nsIImageLoadingContent within the contentSecurityManager we could call ::SetBLockedRequest() on it within the contentSecurityManager. That we we wouldn't have to introduce the new error codes and propagate them up the stack. But as you said, not sure how fragile that logic is.

> but theoretically we could only propagate the new errors up the callstack > if the content type is TYPE_*_IMAGE. Ah, that would help mitigate the risk, yes. > but in contract to ::CanLoadImage, where we pass the this pointer as the context > (namely the nsIImageLoadingContent), we pass |aDocument| to ::LoadImage() You're going to need to solve this problem anyway, because existing content policies for images expect access to the HTMLImageElement involved.

Comment on attachment 8728683 [details] [diff] [review] bug_1206961_asyncopen_imgloader_layout_changes.patch >+++ b/layout/xul/nsImageBoxFrame.cpp You lost the null-check for "uri". And it looks like you're changing what node is used as the context for content policy checks, yes? Maybe that's ok... >+++ b/layout/xul/tree/nsTreeBodyFrame.cpp Again, this is changing the node used as context. r=me modulo those issues, I think.

I'd much prefer that we return specific error codes than hack image-specific callbacks into our networking code. Such callbacks would be quite fragile and make other development harder I bet. I'd still love to understand what we're doing differently based on how the image load is blocked.

(In reply to Jonas Sicking (:sicking) from comment #28) > I'd much prefer that we return specific error codes than hack image-specific > callbacks into our networking code. Such callbacks would be quite fragile > and make other development harder I bet. Ok, makes sense. Let's try to do this and create new ERROR codes for: const short REJECT_REQUEST = -1; const short REJECT_TYPE = -2; const short REJECT_SERVER = -3; const short REJECT_OTHER = -4; > I'd still love to understand what we're doing differently based on how the > image load is blocked. In the current code (without the patches from this bug applied) we are performing security checks and call SetBlockedRequest using the actual contentPolicy decision as an argument (see Comment 21 for details). Now, that we have security checks within asyncOpen2() we don't currently propagate the contentPolicy decision up the stack. We only return NS_ERROR_CONTENT_BLOCKED and hence can't call SetBlockedRequest() with the correct argument.

> > I'd still love to understand what we're doing differently based on how the > > image load is blocked. > > In the current code (without the patches from this bug applied) we are > performing security checks and call SetBlockedRequest using the actual > contentPolicy decision as an argument What I'm asking is what does SetBlockedRequest do with that value? Why does it do it. This is a question for bz, or someone that knows imglib history well.

> I'd still love to understand what we're doing differently based on how the image load is blocked. The basic issue is that we use content policies to implement a variety of different "block the image" scenarios. All of the following are implemented as content policies: 1) Disabling images across the board. See the permissions.default.image (nee network.image.imageBehavior) preference. 2) Disabling images for a particular host. You can do this via page info for example. 3) Disabling a particular image via things like adblock plus. We wanted different behavior for these cases. For example, if images are disabled across the board, we wanted to show alt text instead. If images from a particular host are disabled, we wanted to just make them act as if they were display:none. For a single blocked image, adblock plus wanted UI to allow loading the image, that sort of thing. So at some point we decided to create specific states that would reflect the semantic meaning of stuff like this. These are mapped into CSS as the :-moz-user-disabled, :-moz-suppressed, and :-moz-broken pseudo-classes. We map REJECT_TYPE to :-moz-user-disabled, REJECT_SERVER to :-moz-suppressed and anything else to :-moz-broken (which is also what happend if the image just has invalid data, say). We then have styles in html.css that set :-moz-suppressed things to be display:none. We have rules in html.css that set :-moz-broken and :-moz-user-disabled to show the alt text. We have code in nsImageFrame::ShouldCreateImageFrameFor that examines the state of the image and decides whether we should create an nsImageFrame or a frame by display. It looks at this point like :-moz-user-disabled and :-moz-broken are generally treated similarly at least in our code. Extensions and user stylesheets may be treating them differently. :-moz-suppressed is treated quite differently, per the html.css rules above. By the way, CSP uses REJECT_SERVER, so ends up in the :-moz-suppressed case (and hence never shows alt text). Not clear to me whether that's intentional. But the change of behavior for the REJECT_SERVER case is what caused at least one of Christoph's test failures.

Comment on attachment 8728661 [details] [diff] [review] bug_1206961_imgloader_asyncopen2_image_changes.patch Review of attachment 8728661 [details] [diff] [review]: ----------------------------------------------------------------- Based on your responses, this is OK with me, but I'd really like someone more familiar with the security aspects of this change to review as well. Boris, since you've got this stuff paged in, would you mind taking a quick look?

> The basic issue is that we use content policies to implement a variety of > different "block the image" scenarios. All of the following are implemented > as content policies: > > 1) Disabling images across the board. See the permissions.default.image > (nee network.image.imageBehavior) > preference. > 2) Disabling images for a particular host. You can do this via page info > for example. > 3) Disabling a particular image via things like adblock plus. > > We wanted different behavior for these cases. For example, if images are > disabled across the board, we wanted to show alt text instead. If images > from a particular host are disabled, we wanted to just make them act as if > they were display:none. For a single blocked image, adblock plus wanted UI > to allow loading the image, that sort of thing. Ah! That is super helpful! So this is actually why I thought that we should pass the loading*node* as argument to nsILoadInfo, and not just the loading*document*. My thinking was that individual content policies could grab the <img> that is doing the load and modify its behavior/state or stick it in a hash, etc, to affect how the returned resource is handled, how a blocked load is handled. So, could we do this: Could we make the code that implement 1 and 2 call some function on the <img> element to set its block state. I.e. instead of generically mapping results from *all* nsIContentPolicies into :-moz-user-disabled, :-moz-suppressed, and :-moz-broken, we instead make the specific policies that implement 1 and 2 above call into the image element and set the block state. I'm not sure of the details of how 3 is done currently. Obviously we can't rewrite adblock plus ourselves. Does adblock plus currently rely on that some specific nsIContentPolicy block state is mapped to a specific CSS pseudo-class?

> Could we make the code that implement 1 and 2 call some function on the <img> element to set its block state. We could. We would need to hunt down the relevant bits of code (e.g. which behavior does CSP want?) and modify them. Might need to add some scriptable chromeonly accessors on the image loading content, possibly. Note that this is precisely the second option from comment 24, no? > I'm not sure of the details of how 3 is done currently. I think the main point was that for 3 adblock plus wanted a way to block an image that would not make it disappear entirely from the display. I don't know whether it ends up relying on the pseudo-classes we set up right now, but we could presumably check (e.g. look for REJECT_TYPE and REJECT_SERVER in addons).

> Note that this is precisely the second option from comment 24, no? Ah, I had understood it as something slightly different. I think that if policies explicitly call into functions only when we know that we want to change state, I think it'll be fine. > > I'm not sure of the details of how 3 is done currently. > > I think the main point was that for 3 adblock plus wanted a way to block an > image that would not make it disappear entirely from the display. I don't > know whether it ends up relying on the pseudo-classes we set up right now, > but we could presumably check (e.g. look for REJECT_TYPE and REJECT_SERVER > in addons). Sounds good. We should definitely check that we don't break adblock plus.

(In reply to Boris Zbarsky [:bz] from comment #26) > > but in contract to ::CanLoadImage, where we pass the this pointer as the context > > (namely the nsIImageLoadingContent), we pass |aDocument| to ::LoadImage() > > You're going to need to solve this problem anyway, because existing content > policies for images expect access to the HTMLImageElement involved. Boris, Jonas, before implementing that I wanted to make sure we all agree on the approach. Within the ContentSecurityManager we basically do: > @@ -298,16 +300,32 @@ DoContentSecurityChecks(nsIURI* aURI, ns > aURI, > aLoadInfo->LoadingPrincipal(), > requestingContext, > mimeTypeGuess, > nullptr, //extra, > &shouldLoad, > nsContentUtils::GetContentPolicy(), > nsContentUtils::GetSecurityManager()); > + > + // for images we have to do something special and fire the > + // SetBlockedReuqest > + if (contentPolicyType == nsIContentPolicy::TYPE_IMAGE) { > + // > + nsCOMPtr<nsIImageLoadingContent> imgLoadingContent = > + do_QueryInterface(requestingContext); > + > + if (imgLoadingContent) { > + imgLoadingContent->SetBlockedRequest( > + aURI, > + NS_FAILED(rv) ? nsIContentPolicy::REJECT_REQUEST, > + : shouldLoad); > + } > + } The question that remains for me. Within nsImageLoadingContent.cpp we call nsContentUtils::CanLoadImage() using the |this pointer| as the context, but we call nsContentUtils::LoadImage() using |aDocument| as the loadingDocument. I suppose we still want to keep the loadingDocument because it's then passed as the loadingNode to NS_NewChannel(). Now the question is, how should we pass the nsImageLoadingContent to the loadInfo so we can query it from within the contentSecurityManager. Or is there some magic available so I can query the nsImageLoadingContent without modifying the argument passed to ::LoadImage()? I hope I am not missing anything else. Potentially we should also update nsCSPContext and not always return REJECT_SERVER for all the error cases.

We need the document somewhere under LoadImage because imagelib uses it as part of its cache key. I don't think there is any magic here; we'll need to change what's passed to nsContentUtils::LoadImage, and probably the imagelib API, if we want to thread the image node down to where we apparently want the content policy check.

No. We should not make any changes to nsContentSecurityManager.cpp We should then do something like: rv = channel->AsyncOpen2(...); if (NS_FAILED(rv) && NS_CP_ACCEPTED(mImageBlockingStatus)) { SetBlockedRequest(uri, nsIContentPolicy::REJECT_REQUEST); // Might want to pick a different default. bz? } Then move the SetBlockedRequest function to the nsIImageLoadingContent interface. And then in the various content-policies that we have for image blocking, have the policy do if (<...logic for blocking...>) { nsCOMPtr<nsIImageLoadingContent> img = do_QueryInterface(aRequestContext); img->SetBlockedRequest(aURI, REJECT_TYPE); // Or REJECT_SERVER if that's what is returned now } At least that's the rough outline. I.e. the imageloading code itself just uses a default policy for if a load is blocked. For content-policies that want another policy, they call into the element and set state on the element to tell it to block in a different way.

Ok, thanks. So as a first step I am going to file a new bug (blocking this on) where we just add another argument |nsISupports* aContext| to LoadImage ... > +nsContentUtils::LoadImage(nsIURI* aURI, nsISupports* aContext, > + nsIDocument* aLoadingDocument, ... and all the way down to NewImageChannel(), so we can call NS_NewChannel() using the context which is in the case of nsImageLoadingContent.cpp > + nsresult rv = nsContentUtils::LoadImage(aNewURI, > + static_cast<nsIImageLoadingContent*>(this), > + aDocument,

Comment on attachment 8728661 [details] [diff] [review] bug_1206961_imgloader_asyncopen2_image_changes.patch So we used to pass different policy types to NS_CheckContentLoadPolicy and nsMixedContentBlocker::ShouldLoad. Why was that, and why are we changing it? This is the sort of thing that should have been in the commit message if it's purposeful. >+ /* aInheritForAboutBlan */ false, "Blank" lost its 'k'. Also, this construct: *aForcePrincipalCheckForCacheEntry = aLoadingPrincipal ? stuff : false; is probably simpler to follow as: *aForcePrincipalCheckForCacheEntry = aLoadingPrincipal && stuff; >@@ -2335,19 +2306,27 @@ imgLoader::LoadImageWithChannel(nsIChann ... >- nsIContentPolicy::TYPE_INVALID, false, nullptr, >+ policyType, false, nullptr, Why this change to the ValidateEntry call? Again, an explanation in the commit message about why the old code is wrong would be a good idea....

Carrying over r+ from Seth.

Comment on attachment 8742986 [details] [diff] [review] bug_1206961_asyncopen_imgloader_image_changes.patch (In reply to Boris Zbarsky [:bz] from comment #40) > >- nsIContentPolicy::TYPE_INVALID, false, nullptr, > >+ policyType, false, nullptr, > > Why this change to the ValidateEntry call? Again, an explanation in the > commit message about why the old code is wrong would be a good idea.... Hey Boris, ValidateEntry calls ValidateSecurityInfo which calls ShouldLoadCachedImage which calls NS_CheckContentLoadPolicy which internally calls all nsIContentPolicies. For example, MixedContentBlocker asserts that the passed contentpolicyType is valid (in other words, not TYPE_INVALID). I think what happenend is that one of the nsContentUtils::CanLoadImage() checks already blocked the load before we got to this point. Now that we are removing nsContentUtils::CanLoadImage() we hit this codepath, hence we should pass the right contentPolicyType.

Boris, Jonas, unfortunately I can't completely follow what Jonas suggests within Comment 38. In my opinion it would make the most sense if we call SetRequestBlocked() within nsContentPolicy.cpp. But then we also have to call it within nsContentSecurityManger anyway because nsContentUtils::CanLoadImage() also calls CheckLoadURIWithPrincipal() [1]. Further I qfolded the changes from layout/ (which were already r+ed) and dom/ into this single changeset. One more question about nsCSPContext.cpp where we always used to return REJECT_SERVER which also affects SetBLockedRequest(), should we simply return REJECT_REQUEST or do we have to update more than that? [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#3022

Christoph, can you please post an interdiff from the last thing I reviewed here?

Also, my first question from comment 40 did not get answered. And again, this sort of thing should be in the commit message of the patch...

Comment on attachment 8742987 [details] [diff] [review] bug_1206961_asyncopen_imgloader_loadimage_updates.patch I don't think we need to change behavior in nsCSPContext.cpp. Certainly not in this bug. But what I really want to know here is whether the current invariant that SetBlockedRequest is called precisely when PrepareNextRequest() would be called if we were not blocked is being maintained. Also, this looks like a diff against ... not trunk, right? Is there a diff against trunk that would let me see what the cumulative changes to nsImageLoadingContext and its consumers is? Basically, I'm willing to review this stuff, but paging it back in after a month with no clear indication of how the patches fit together, no clear commit messages, and no clear description of exactly what the logic flow is in the new setup and why it's equivalent to the old setup is ... painful. I do _not_ have a full day or so just to deal with this review. :(

Comment on attachment 8742987 [details] [diff] [review] bug_1206961_asyncopen_imgloader_loadimage_updates.patch Er, nevermind. Looks like this is a diff against trunk; I missed (or forgot) that we moved nsContentUtils::CanLoadImage into nsImageLoadingContent::LoadImage. The rest of my comments stand. :(

And in particular, how does the new setup compare to the setup we used to have when we called CanLoadImage outseide LoadImage? How did the SetBlockedRequest stuff work at that point?

(In reply to Boris Zbarsky [:bz] from comment #40) > So we used to pass different policy types to NS_CheckContentLoadPolicy and > nsMixedContentBlocker::ShouldLoad. Why was that, and why are we changing > it? This is the sort of thing that should have been in the commit message > if it's purposeful. Sorry for not answering this question earlier. I thought it's the same question as why we are not using TYPE_INVALID anymore, but in fact it isn't. The fact that we used to pass two different contentPolicy types to NS_CheckContentLoadPolicy and nsMixedContentBlocker::ShouldLoad() is simply wrong in my opinion. In both cases we should have passed TYPE_INTERNAL_IMAGE. I think there is no need for an interdiff, becuase nothing changed since you last review. I just answered your questions. The only change I incorporated is: - *aForcePrincipalCheckForCacheEntry = inherit && !isSandBoxed; + // only inherit if we have a principal + *aForcePrincipalCheckForCacheEntry = + aLoadingPrincipal && + nsContentUtils::ChannelShouldInheritPrincipal( + aLoadingPrincipal, + aURI, + /* aInheritForAboutBlank */ false, + /* aForceInherit */ false);

(In reply to Boris Zbarsky [:bz] from comment #46) > Basically, I'm willing to review this stuff, but paging it back in after a > month with no clear indication of how the patches fit together, no clear > commit messages, and no clear description of exactly what the logic flow is > in the new setup and why it's equivalent to the old setup is ... painful. I > do _not_ have a full day or so just to deal with this review. :( I understand that, and I am sorry for the long delay. I suppose I need some time to answer all of your questions. Let me get back to you later this week with more answers. Thanks for taking the time to help me move this bug forward.

Comment on attachment 8742987 [details] [diff] [review] bug_1206961_asyncopen_imgloader_loadimage_updates.patch Just chatted with Jonas over vidyo and I finally understand what he means within Comment 38. I need to update those bits and will ask for review again shortly.

> I think there is no need for an interdiff, becuase nothing changed since you last review. That right there is useful information. ;)

Oh, and I realized just now that of course the CanLoadImage in image loading content must have been there all along. Sorry for the confusion about that. :(

(In reply to Boris Zbarsky [:bz] from comment #46) Boris, again sorry for all the confusion and the long silence within this bug. I chatted with Jonas yesterday and I updated the patch according to this feedback. Basically we are implementing the algorithm Jonas' suggested within comment 38. I flagged both of you for feedback. If you are timely constrained (which I know you are ;-)) you can let Jonas do the first round of feedback, since we chatted in Person yesterday and he is already familiar with the idea. Besides the following question, I suppose all of your questions should have been answered by now. Otherwise let me know. > But what I really want to know here is whether the current invariant that > SetBlockedRequest is called precisely when PrepareNextRequest() would be > called if we were not blocked is being maintained. I would say yes. On current mozilla-central we are calling SetBlockedRequest() if CanLoadImage() fails [1]. Within the new setup, specific contentPolcies which do not return the default REJECT_* value when blocking the load call SetBlockedRequest() within shouldLoad if the requestingContext is in fact an nsImageLoadingContent. Later, on the callsite of AsyncOpen2() we potentially have to call SetBlockedRequest() if AsycnOpen2() failed *and* none of the contentPolicies already called SetBlockedRequest, which basically means CheckLoadURIWithPrincipal() blocked the load. The main question that remains: we need to be careful so we don't forget to update an implementation of nsIContentPolicy which needs to call SetBlockedRequest. Following a list of all classes that implement nsIContentPolicy, +++ denotes that I updated the policy to call SetBlockedRequest: /dom/base/nsContentPolicy.h +++ /dom/base/nsDataDocumentContentPolicy.h /dom/base/nsNoDataProtocolContentPolicy.h /dom/security/nsCSPService.h /dom/security/nsMixedContentBlocker.h +++ /embedding/browser/nsWebBrowserContentPolicy.h /extensions/permissions/nsContentBlocker.h /toolkit/mozapps/extensions/AddonContentPolicy.h One last note, IsAppTypeEditorForImageLoads() is basically a copy of nsContentUtils::CanLoadImage() [2] to make sure CheckLoadURIWithPrincipal() checks are bypassed for APP_TYPE_EDITOR. [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#849 [2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#3011

Comment on attachment 8743297 [details] [diff] [review] bug_1206961_asyncopen_imgloader_loadimage_updates.patch Review of attachment 8743297 [details] [diff] [review]: ----------------------------------------------------------------- What did you search the tree for when deciding where to insert calls to SetRequestBlocked? ::: image/imgLoader.cpp @@ +1698,5 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { > + // If AsyncOpen2() fails, we have to let the ImageLoadingContent > + // know that the load was blocked. Please note that specific > + // contentPolicies might already have called SetBlockedRequest > + // using a specific REJECT_* value. In such cases we do not want This code should probably live in nsImageLoadingContent.cpp, right after the call to nsContentUtils::LoadImage. That's where the call to SetBlockedRequest used to live, and so would be most consistent with what we used to do.

(In reply to Jonas Sicking (:sicking) from comment #56) > What did you search the tree for when deciding where to insert calls to > SetRequestBlocked? Searched for "public nsIContentPolicy": http://mxr.mozilla.org/mozilla-central/search?string=public%2BnsIContentPolicy > ::: image/imgLoader.cpp > @@ +1698,5 @@ > > if (NS_WARN_IF(NS_FAILED(rv))) { > > + // If AsyncOpen2() fails, we have to let the ImageLoadingContent > > + // know that the load was blocked. Please note that specific > > + // contentPolicies might already have called SetBlockedRequest > > + // using a specific REJECT_* value. In such cases we do not want > > This code should probably live in nsImageLoadingContent.cpp, right after the > call to nsContentUtils::LoadImage. That's where the call to > SetBlockedRequest used to live, and so would be most consistent with what we > used to do. Yes, I agree, we should move it back there and remove it from the two places right after newChannel->asyncOpen2().

Updated failing tests. (This TRY push already includes the changes from comment 56 where I moved SetBlockedRequest into nsImageLoadingContent.cpp: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b38bc379a47f

Comment on attachment 8743796 [details] [diff] [review] bug_1206961_asyncopen_imgloader_test_updates.patch Wait. Why are we still changing test_docwrite_meta.html? The whole point earlier was that this test caught the bug about the content policy state not being correctly propagated to the element state! Please undo that change. In general, please don't make changes to tests unless you _know_ the test is buggy and clearly explain why in the commit message. >+ // the created document does not have a loadgroup, which >+ // is asserted within nsContentUtils::LoadImage() Well, why did this not use to be a problem? Is the assert bogus? Did it use to be bogus? Oh, wait. I mentioned this all already in comment 12. Please just address the earlier review comments... Please do not check this patch in as-is.

Might be easier to search for policies which use other REJECT types than REJECT_REQUEST: http://mxr.mozilla.org/mozilla-central/ident?i=REJECT_OTHER We don't have any of these apparently. http://mxr.mozilla.org/mozilla-central/ident?i=REJECT_SERVER This is used by CSP and extensions/permissions/nsContentBlocker.cpp CSP likely doesn't want to use special image-blocking behavior. nsContentBlocker.cpp seems like it's using special REJECT_* quite intentionally, so I'd add a call to SetBlockedRequest here for image loads. http://mxr.mozilla.org/mozilla-central/ident?i=REJECT_TYPE I'll let you audit this one.

Comment on attachment 8742986 [details] [diff] [review] bug_1206961_asyncopen_imgloader_image_changes.patch It's probably worth adding a comment in NewImageChannel that explains that aLoadingPrincipal is actually the triggering principal in loadinfo terms. r=me

Comment on attachment 8743297 [details] [diff] [review] bug_1206961_asyncopen_imgloader_loadimage_updates.patch OK, I have this completely paged in again now. The bad news is that there are tons of content policies that (1) return REJECT_SERVER and (2) are not under our control. See https://mxr.mozilla.org/addons/search?string=REJECT_SERVER (not all hits are returns from content policies, but quite a number of them are). This patch would break them all, as far as I can tell. Furthermore, it's not even adding a way they could unbreak themselves. :( I did mention in comment 34 that we should do this search before deciding what we're doing here... Anyway, the way I see it, we have three options, in rough order of desirability from my point of view, most desirable first: 1) Preserve compat with these addons. That means that the call to SetBlockedRequest needs to happen at whatever central place it is that invokes content policies in this case. Yes, I know Jonas has been pushing away from this approach all along. I don't understand why he thinks this is the wrong thing to do. I won't claim he's wrong until I _do_ understand it, because maybe I'm missing something critical here, but at first glance it's because he thinks it's cleaner architecture. If so, we need to weigh that against the compat breakage, at least. 2) Break compat with the addons, provide a way they can fix things. That means adding setBlockedRequest to the MozImageLoadingContent mixin in HTMLImageElement.webidl and then probably involves people contacting these addon authors and getting them all to make boilerplaty changes to their code. They will probably hate us, and I think they will be right to do so. ;) 3) Just break compat, don't provide a way to unbreak. I think this is a bad idea.

Mainly I wanted an architecture which was more flexible and more explicit. I bet that a lot of addons that return REJECT_SERVER do it because they copy-pasted the code, or because that description sounded most accurate in nsIContentPolicyBase.idl, rather than that they wanted a specific image-blocking behavior. The CSP code seems to be an example of this. That said, I keep making the mistake of trying to clean up nsIContentPolicy, and the reality is that it's just not worth it. So I guess what we should do is that in DoContentSecurityChecks (in nsContentSecurityManager.cpp)add the code that checks the policy REJECT value and the contentpolicy-type. If we're loading an image, and the reject-type is something other than REJECT_REQUEST, then we do the QI and call SetBlockedRequest. Alternatively, rather than doing this in DoContentSecurityChecks we could do it in NS_CheckContentLoadPolicy if that's preferred. Though that might be a riskier change. Does that sound good?

> and the reality is that it's just not worth it. Yep. :( The proposal sounds good if all the content policy stuff goes through DoContentSecurityChecks.

(In reply to Boris Zbarsky [:bz] from comment #64) > The proposal sounds good if all the content policy stuff goes through > DoContentSecurityChecks. Conceptually I agree that we should call SetBlockedRequest within DoContentSecurityChecks within the contentSecurityManager. I am slightly afraid that we are going to miss a cornercase, e.g. images loaded from the cache do not go through DoContentSecurityChecks() but we have to perform a call to NS_CheckContentLoadPolicy for them. And just to be precise, we do call NS_CheckContentLoadPolicy() when loading cached images. Hence, I suppose the better place, especially to maintain parity between the old and the new implementation would be to call SetBlockedRequest() within NS_CheckContentLoadPolicy. And in addition we should keep the call to SetBlockedRequest() within nsImageLoadingContent in case the load was blocked by some other security check than contentpolicies. Do we all agree?

If we all agree, then this would be the changeset we are shooting for. Regarding the tests, the CSP test works with that patch. The failing assertion within LoadImage() for the other tests need a little more investigation. Once we agree on this patch, I will debug those tests again and flag you for review.

Comment on attachment 8744387 [details] [diff] [review] bug_1206961_asyncopen_imgloader_loadimage_updates.patch If we can spin this off into a separate bug without all the confusing discussion about the other other patches, that would be a _really_ good idea.... >+++ b/dom/base/nsContentPolicy.cpp Can we limit the QIing to cases when we have an appropriate content type? Seems like it might be a tad faster. >+++ b/dom/base/nsImageLoadingContent.cpp >@@ -878,54 +864,64 @@ nsImageLoadingContent::LoadImage(nsIURI* You're changing the behavior here. We used to ForgetImagePreload no matter whether the LoadImage call succeeded or not. Please continue to do so. Slightly more complicated: you're changing the behavior of what gets stored in mCurrentRequestFlags. In the old setup, if content policy blocked a load attempt while !HaveSize(mCurrentRequest), then mCurrentURI would get set to the new URI, but mCurrentRequestFlags would keep the REQUEST_IS_IMAGESET fag precisely if the _old_ current request had it. This seems odd, but that was the behavior. In the new setup, you're going to call either PreparePendingEquest or PrepareCurrentRequest before the SetBlockedRequest calls happen. If you PreparePendingRequest, then I think we have the same behavior we used to. But if you PrepareCurrentRequest (which happens precisely in the !HaveSize(mCurrentRequest) case, afaict) that will clobber mCurrentRequestFlags with the ones for the _new_ load and then we'll end up with the new load's REQUEST_IS_IMAGESET flag, or lack thereof. This might be fine, and in fact may even be desirable, but please check with whoever added this flag to see why the behavior right now is what it is. There are other similar issues too. For example, consider the case when we enter LoadImage with a non-null mCurrentRequest but with !HaveSize(mCurrentRequest) and we're trying to load something that gets blocked. In the old setup, we would call SetBlockedRequest, which would clear it with NS_ERROR_IMAGE_BLOCKED. See the XXX(seth) comments in SetBlockedRequest about the exact nsresult mattering. In the new setup, we would call PrepareCurrentRequest() first, which would ClearCurrentRequest(NS_ERROR_SRC_CHANGED). Our later call to ClearCurrentRequest from inside SetBlockedRequest would no-op because at that point mCurrentRequest is null. Or another: In the old setup, a failure return from AsyncOpen did NOT set mImageBlockingStatus to a rejected status. In particular, if the image got moved in the DOM after such a failure, we used to take the "URI equality check" codepath in LoadImage, and since the URI would match and mImageBlockingStatus would be accepted we would skip retrying the load. In the new setup it looks to me like we'll retry the load in that situation. Maybe that's OK, but it's not obvious to me that this is an intentional behavior change. In general, I'd like an analysis here of all the cases in which this patch is changing behavior, with an explanation for each one of why the change is desirable. I didn't keep auditing to see whether there are more differences, because I'm not sure the logic flow here will even remain the same.... >+ int16_t blockingStatus = nsIContentPolicy::ACCEPT; >+ static_cast<nsIImageLoadingContent*>(this)-> >+ GetImageBlockingStatus(&blockingStatus); That's a complicated way to write this: int16_t blockingStatus = ImageBlockingStatus(); >+ if (NS_CP_ACCEPTED(blockingStatus)) { >+ static_cast<nsIImageLoadingContent*>(this)-> >+ SetBlockedRequest(aNewURI, nsIContentPolicy::REJECT_REQUEST); Why do we actually want to do that here? This is what's causing one of the above behavior differences, but why do we need this call now that we've centralized the SetBlockedRequest() behavior in content policy? I guess that's needed for the CheckLoadURI case, basically? Since we're adding image special-casing anyway in nsContentSecurityManager.cpp can we just do the call there to reduce the number of behavior changes we're making? If not, we need to think a bit extra about this stuff. The rest of the changes look good.

(In reply to Boris Zbarsky [:bz] from comment #67) > If we can spin this off into a separate bug without all the confusing > discussion about the other other patches, that would be a _really_ good > idea.... Yes, indeed that is a good idea, I filed Bug 1267075 where we can handle all changes related to nsImageLoadingContent. I suppose we can land patches from this bug and then follow up on nsImageLoadingContent. Since we then would keep nsContentUtils::CanLoadImage() within nsImageLoadingContent I suppose that should be ok, right? > The rest of the changes look good. Only the rest remains within that patch. Let's see if there is still something missing we have to update: https://treeherder.mozilla.org/#/jobs?repo=try&revision=551825bd963e

> I suppose that should be ok, right I _think_ so, yes.

Boris, first, thanks for all your help and guidance with that bug. Second, as agreeed all the changes related to nsImageLoadingContent will be handled within Bug 1267075. And third, I also had to update layout/xul/nsImageBoxFrame.cpp to only call LoadImage if the uri to be loaded is not a nullptr. Everything else in this changeset remains the same to what we have discussed previously.

Hey Ben and Ehsan, converting the imgLoader to use AsyncOpen2() would requite us to update the following tests: @Ben, fetch-canvas-tainting.https.html and fetch-canvas-tainting-cache.https.html log the following error to the console: Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://www1.web-platform.test:8443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE&ACAOrigin=https://web-platform.test:8443. (Reason: expected 'true' in CORS header 'Access-Control-Allow-Credentials'). Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://www1.web-platform.test:8443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE&ACAOrigin=https://web-platform.test:8443. (Reason: expected 'true' in CORS header 'Access-Control-Allow-Credentials'). I am not precisely sure where the difference lies, but I assume since the implementation of fetch [1] is split between Necko and the CorsListenerProxy that there might be a bug in the current fetch implementation that asyncOpen2() fixes. Do we need to investigate further or do you have an idea what might be wrong and you would agree to updating the test. The other failing test is fetch-request-css-images.https.html. Currently, on mc the test fails because * css_image_test(f, LOCAL_URL, 'backgroundImage', 'no-cors', 'include'); already returns an error. Now that we are using asyncOpen2() for image loads, the first two tests: * css_image_test(f, LOCAL_URL, 'backgroundImage', 'no-cors', 'include'); * css_image_test(f, REMOTE_URL, 'backgroundImage', 'no-cors', 'include'); And the third one: css_image_test(f, LOCAL_URL, 'shapeOutside', 'cors', 'same-origin'); causes the test to time out. The timeout is unrelated to the changes within this bug. On current central the third test also causes the test to time out if I comment out the first two tests. I suppose updating the *.ini to indicate the test is expected to time out is the right change for this bug. Agreed? @Ehsan, similar to lines we updated for test fetch-request-resources.https.html within Bug 1195173, we have to update 'omit' to 'same-origin' within this bug. Further, we can enable some more tests. [1] http://hg.mozilla.org/mozilla-central/annotate/ab8a76ac7b34/dom/fetch/FetchDriver.cpp#l104

Comment on attachment 8744824 [details] [diff] [review] bug_1206961_update_web_platform_tests.patch Review of attachment 8744824 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/meta/service-workers/service-worker/fetch-request-css-images.https.html.ini @@ +1,4 @@ > [fetch-request-css-images.https.html] > type: testharness > [Verify FetchEvent for css images.] > + expected: TIMEOUT Isn't TIMEOUT what we use to indicate an expected timeout? Why is this still failing? https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e713c276d48

Looking at that test diff for testing/web-platform/tests/service-workers/service-worker/resources/fetch-canvas-tainting-iframe.html: > create_test_promise( > image_url + > '&mode=cors&url=' + > encodeURIComponent(remote_image_url + > '&ACAOrigin=' + host_info['HTTPS_ORIGIN']), > '', >- NOT_TAINTED), >+ LOAD_ERROR), // We expect LOAD_ERROR since the server doesn't respond >+ // with an Access-Control-Allow-Credentials header. This is passing '' as the "cross_origin" argument to create_test_promise, which afaict should not set the "crossOrigin" attribute on the image in create_test_case_promise, right? Why would this even do CORS at all? Shouldn't this just be a no-CORS same-origin load? I mean, the document itself is same-origin with image_url! Similar for the other test you modified in that file: it's doing a no-CORS cross-origin load, so should succeed but taint. If those tests are failing, that seems like a bug in the imgLoader changes to me that this patch is introducing, not a preexisting bug that it's fixing...

Comment on attachment 8744824 [details] [diff] [review] bug_1206961_update_web_platform_tests.patch Review of attachment 8744824 [details] [diff] [review]: ----------------------------------------------------------------- Please add test cases setting credentials=same-origin in fetch-canvas-tainting-iframe.html. In that case I would expect both of these two new cases to load as UNTAINTED since the service worker gets CORS responses. Also, we need to understand why fetch-request-css-images.https.html is timing out now. ::: testing/web-platform/tests/service-workers/service-worker/resources/fetch-canvas-tainting-iframe.html @@ +210,5 @@ > encodeURIComponent(remote_image_url + > '&ACAOrigin=' + host_info['HTTPS_ORIGIN']), > '', > + LOAD_ERROR), // We expect LOAD_ERROR since the server doesn't respond > + // with an Access-Control-Allow-Credentials header. I think this is correct. The service worker is creating a new fetch() with mode cors. It uses the original no-cors request's credentials value of "include". So since the server does not return Access-Controll-Allow-Credentials this should fail to load. It would be nice to add another test case for each of these that sets "credentials=same-origin" in the image_url params to test the original case, though.

Comment on attachment 8744823 [details] [diff] [review] bug_1206961_asyncopen_imgloader_loadimage_updates.patch >+bool IsAppTypeEditorForImageLoads(nsILoadInfo* aLoadInfo) { I'd call this IsImageLoadInEditorAppType. And it should be static. And curly on next line, please. >+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(aLoadInfo->LoadingNode()); Will this do the right thing? What's aLoadInfo->LoadingNode() in the <img> case? Is it the document, or the <img> element? >+ !IsAppTypeEditorForImageLoads(aLoadInfo)) { The common case is loads that are not nsIContentPolicy::TYPE_DOCUMENT, so I think you should call IsImageLoadInEditorAppType(aLoadInfo) once and save the result in a boolean, then check it in the two if statements. And please don't lose the comments that CanLoadImage has which explain WHY we are doing this bizarre app type check. r=me modulo that question about QI to nsIDocument above. Do we have test coverage for that codepath with <img>?

Comment on attachment 8744824 [details] [diff] [review] bug_1206961_update_web_platform_tests.patch Review of attachment 8744824 [details] [diff] [review]: ----------------------------------------------------------------- I haven't looked closely at this but seems like we need answers to comment 74 and 75 here.

Comment 74 is probably wrong if the service worker really is doing a CORS load no matter what the original load was. But I'd like to undestand why that CORS load wasn't working correctly (i.e. failing) before now. What credentials value did we end up using and why?

(In reply to Boris Zbarsky [:bz] from comment #78) > Comment 74 is probably wrong if the service worker really is doing a CORS > load no matter what the original load was. But I'd like to undestand why > that CORS load wasn't working correctly (i.e. failing) before now. What > credentials value did we end up using and why? We were passing the wrong credentials mode before. We were waiting for AsyncOpen2 to fix this by using the new credentials policy flags. So the change is expected. I can get links and whatnot later this morning if you like. I'm on my mobile right now.

(In reply to Boris Zbarsky [:bz] from comment #76) > >+bool IsAppTypeEditorForImageLoads(nsILoadInfo* aLoadInfo) { > ... > >+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(aLoadInfo->LoadingNode()); > > Will this do the right thing? What's aLoadInfo->LoadingNode() in the <img> > case? Is it the document, or the <img> element? In some cases we pass the document and in some cases we pass nsIContent. So I updated that piece of code to, first check for the document and if that is not the case, then to query the ownerDoc() of the content. + uint32_t appType = nsIDocShell::APP_TYPE_UNKNOWN; + nsCOMPtr<nsIDocument> doc = do_QueryInterface(aLoadInfo->LoadingNode()); + if (!doc) { + nsCOMPtr<nsIContent> content = do_QueryInterface(aLoadInfo->LoadingNode()); + if (content) { + doc = content->OwnerDoc(); + } + if (!doc) { + return false; + } + } Given that the previous patch already passed TRY, I suppose we don't have any test coverage for that code path. If you could sketch out a simple test, I am happy to add it to our codebase.

> We were passing the wrong credentials mode before. Ah, ok. No need for links.

(In reply to Ben Kelly [:bkelly] from comment #75) > It would be nice to add another test case for each of these that sets > "credentials=same-origin" in the image_url params to test the original case, > though. I don't fully understand what test you would like to have added. Is it: create_test_promise( image_url + '&mode=same-origin&url=' + encodeURIComponent(remote_image_url), 'use-credentials', NOT_TAINTED), If that is the case, then we would still get LOAD_ERROR and would see the following error in the browser console: Security Error: Content at https://web-platform.test:8443/service-workers/service-worker/resources/fetch-rewrite-worker.js may not load data from https://www1.web-platform.test:8443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE. Please let me know what tests you would like to have added and I can add those of course.

> So I updated that piece of code to, first check for the document and if that is not the case, > then to query the ownerDoc() of the content I think you should just do: nsINode* node = aLoadInfo->LoadingNode(); if (!node) { return false; } nsIDocument* doc = node->OwnerDoc(); This will work correctly for both documents and elements without needing separate QIs to check which one you have, because OwnerDoc() on a document returns the document itself (unlike the DOM GetOwnerDocument(), which would return null). > If you could sketch out a simple test I dunno about "simple", but.... I _think_ we could have a browser-chrome test that: 1) Opens a tab. 2) Grabs hold of the nsIDocShell for that tab; with the e10s fun involved. 3) Sets .appType on that nsIDocShell to nsIDocShell::APP_TYPE_EDITOR. 4) Loads some unprivileged (e.g. http://) URI in that tab that contains an <img> pointing to some privileged (file:// or non-exposed chrome:// or something) URI. 5) Checks that the image load succeeds. Steps 2 and 4 are where the non-simplicity is, of course.

(In reply to Boris Zbarsky [:bz] from comment #83) > This will work correctly for both documents and elements without needing > separate QIs to check which one you have, because OwnerDoc() on a document > returns the document itself (unlike the DOM GetOwnerDocument(), which would > return null). Yes, that makes sense to me, thanks. > I _think_ we could have a browser-chrome test Thanks, that might take some time though ...

> Thanks, that might take some time though ... I'm fine with doing the test in a followup; we'd just need to check that the test fails when this function is forced to return false and passes normally. I think we just did a merge, so you have several weeks to get the test in and make sure we're not regressing stuff. That said, at least a manual test would be a good idea once this lands if it doesn't land with the automated test (e.g. building Thunderbird with these changes, or waiting for this patch to merge over there and grabbing a Thunderbird nightly, and ensuring that you can insert file:// images into messages you're composing in HTML mode).

(In reply to Ben Kelly [:bkelly] from comment #75) > Also, we need to understand why fetch-request-css-images.https.html is > timing out now. The first two of tests are passing [1]. Test 3 and 4 try to use the 'shapeOutside' property, which I am not sure is working at all. I tried to update to 'shape-outside' based on [2], but no luck and no error message or indication of an error. Throughout our whole codebase we don't use 'shape-outside' in combination with url(). I am not sure if that is working correctly at all. Test 5,6,7 and 8 all use [3]: > div.style[type] = '-webkit-image-set(url(' + url + ') 1x)'; I also haven't found -webkit-image-set doing and MXR search; I am not sure if that is supposed to be working either. Probably we should keep the first two passing tests, comment out the lower 6 tests and remove the *.ini file. Ben, Boris, what do you think? [1] http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-request-css-images.https.html?force=1#86 [2] https://developer.mozilla.org/en-US/docs/Web/CSS/shape-outside [3] http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/fetch-request-resources-iframe.https.html?force=1#47

We can't comment out tests unfortunately. Our changes here get upstreamed and pulled in by other browsers. Can we factor out the failing cases into separate async_test() or promise_test() calls that fail? Then we could mark them expected fail if we don't implement those CSS properties. That would be the right way to deal with this.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #82) > (In reply to Ben Kelly [:bkelly] from comment #75) > > It would be nice to add another test case for each of these that sets > > "credentials=same-origin" in the image_url params to test the original case, > > though. > > I don't fully understand what test you would like to have added. Is it: > > create_test_promise( > image_url + > '&mode=same-origin&url=' + > encodeURIComponent(remote_image_url), > 'use-credentials', > NOT_TAINTED), I thought I responded to this, but I don't see it in the bug now. I meant to pass the credentials in the URL params so that the service worker uses it for the actual network request. Like: create_test_promise( image_url + '&mode=cors&credentials=same-origin&url=' + encodeURIComponent(remote_image_url), '', NOT_TAINTED),

> Test 3 and 4 try to use the 'shapeOutside' property We don't have support for that property. Chrome seems to be the only browser that does, according to https://developer.mozilla.org/en-US/docs/Web/CSS/shape-outside I guess the test is using shape-outside because it's actually specified to do a fetch with CORS, since it's going to be extracting the image data, so just changing the property used is not OK. Given that, Ben's suggestion in comment 87 is the way to go, I think. > I also haven't found -webkit-image-set doing and MXR search Indeed. This looks like a bug in the test to me in the sense that it's relying on a -webkit property that's not part of <https://compat.spec.whatwg.org/>. There's a proposed standard property called "image-set", but it's in a working draft that hasn't been updated in 3.5 years and isn't obviously going anywhere. I think we should either remove the image-set bits or once again put them in a separate part of the test that we can then annotate.

(In reply to Ben Kelly [:bkelly] from comment #88) > create_test_promise( > image_url + > '&mode=cors&credentials=same-origin&url=' + > encodeURIComponent(remote_image_url), > '', > NOT_TAINTED), The test in the form mentioned above is still missing due to CORS errors, if we want to make it work, the test would have to look like: create_test_promise( image_url + '&mode=cors&credentials=same-origin&ACACredentials=true&url=' + encodeURIComponent(remote_image_url + '&ACAOrigin=' + host_info['HTTPS_ORIGIN']), '', NOT_TAINTED), Ben, is this what you were looking for?

I see we need ACAOrigin, yes, but why do we need ACACredentials=true here? If credentials=same-origin we should not be sending credentials cross-origin. Also, that ACACredentials param won't be interpreted by the service worker and it won't be passed on to the final network request. So I don't think it will have any effect.

(In reply to Ben Kelly [:bkelly] from comment #91) > I see we need ACAOrigin, yes, but why do we need ACACredentials=true here? Indeed, we only need ACAOrigin - I updated that. I further split the several subtests of fetch-request-css-images.https.html into four separate async_test, where only the first one is passing and the other three are annotated to be expected to time out. Please note that I tried to not change the structure of the test too much. I only factored out the common code into a separate function. If you want we could only have two async_test() methods where the first one contains the 2 passing subtests and the other one contains the 6 subtests that are timing out - up to you.

Comment on attachment 8745992 [details] [diff] [review] bug_1206961_update_web_platform_tests.patch Review of attachment 8745992 [details] [diff] [review]: ----------------------------------------------------------------- Thanks so much for splitting the test up! I still don't like the MessageChannel stuff, but its much better now. r=me with a few nits addressed. ::: testing/web-platform/tests/service-workers/service-worker/fetch-request-css-images.https.html @@ +15,3 @@ > > +function css_image_test(expected_results, frame, url, type, > + expexted_mode, expected_credentials) { nit: While you are here can you s/expexted/expected/g? Thanks. @@ +15,5 @@ > > +function css_image_test(expected_results, frame, url, type, > + expexted_mode, expected_credentials) { > + expected_results[url] = { > + url: url, I think we still need to make the urls unique to avoid hitting the image and http caches. I guess just add Date.now()? ::: testing/web-platform/tests/service-workers/service-worker/resources/fetch-canvas-tainting-iframe.html @@ +217,5 @@ > + '&mode=cors&credentials=same-origin&url=' + > + encodeURIComponent(remote_image_url + > + '&ACAOrigin=' + host_info['HTTPS_ORIGIN']), > + '', > + NOT_TAINTED), nit: Please remove trailing whitespace. @@ +255,5 @@ > + '&mode=cors&credentials=same-origin&url=' + > + encodeURIComponent(remote_image_url + > + '&ACAOrigin=' + host_info['HTTPS_ORIGIN']), > + '', > + TAINTED), Please add a comment similar to the previous one here. Something like this uses more fetch spec language: // The cross-origin no-cors request is immediately tainted. Since this // happens before the service worker interception it does not matter // what kind of Response it returns. The result will always be tainted. Also, please fix trailing whitespace here.

Incorporated suggestions from bkelly, carrying over r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e3503205e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/65911fba8069 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf69c3219b5e

https://hg.mozilla.org/mozilla-central/rev/a6e3503205e8 https://hg.mozilla.org/mozilla-central/rev/65911fba8069 https://hg.mozilla.org/mozilla-central/rev/bf69c3219b5e