As a spinoff of bug 1206961, we decided to convert nsImageLoadingContent.cpp to use AsyncOpen2() in a separate bug. Please note that we can convert all the other image loading to rely on asyncOpen2() but we still would call CanLoadImage() within nsImageLoadingContent, so SetBlockedRequest would be called correctly.

(In reply to Boris Zbarsky [:bz] from comment #67) > >+++ 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. Certainly. I considered that actually, but decided to go for the 'less code' version. > >+++ 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. Yes and no. We bailed out early and didn't call ForgetImagePreload() in case a contentPolicy within ::CanLoadImage() blocked the load. Since we are using security checks within asyncOpen2() now, we can't distinguish anymore whether the load was blocked by some contentPolicy, by some other security check or also wehther the actual asynOpen() failed. > 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.... Yes, those are all valid points. > 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. CanLoadImage() basically performed two checks: a) ContentPolicy checks and b) checkLoadURIWIthPrincipal and we used to call SetBlockedReuqest in case CanLoadImage() failed. Now that we have centralized the SetBlockedRequest() call within nsContentPolicy.cpp we could also move SetBlockedRequest into nsContentSecurityManager. Only minor problem, if the image load requires CORS, then the contentSeucityManager does not perform the CheckLoadURIWIthPrincipal check, but rather nsCorsListenerProxy. I don't see that as a big problem, just wanted to mention that explicitly. I think a real parity implementation is not possible anymore due to the change of the security checks setup. The implementation that comes really close though, might be the one in that patch.

Boris, Jonas, let's recap our options real quick to make sure we all agree on the approach for removing the CanLoadImage() check from nsImageLoadingContent. CanLoadImage() basically performed two checks: a) ContentPolicy checks, and b) CheckLoadURIWIthPrincipal() and we used to call SetBlockedRequest within nsImageLoadingContent in case CanLoadImage() failed. [Approach A] (1) Call SetBlockedRequest() within nsContentPolicy.cpp, and also (2) call SetBlockedRequest within nsContentSecurityManager in case CheckLoadURIWIthPrincipal fails. One caveat are CORS loads, because in such a case the ContentSecurityManager does not call CheckLoadURIWIthPrincipal but rather lets nsCorsListenerProxy perform that check (see attached patch). That reminds me that we probably also have to update the CORSListenerProxy to skip the CheckLoadURIWithPrincipal check in case the load comes from a docshell of appType: APP_TYPE_EDITOR. [Approach B] An alternative approach to the one in the attached patch would be to introduce new NS_ERROR_FAILURE_* codes for: * const short REJECT_REQUEST = -1; * const short REJECT_TYPE = -2; * const short REJECT_SERVER = -3; * const short REJECT_OTHER = -4; and then propagate that new NS_ERROR_FAILURE_* up the callstack so we can map it accordingly and still call SetBlockedRequest within nsImageLoadingContent. Both approaches have advantages and disadvantages. E.g. Approach A requires us to call SetBlockedRequest in various different locations. Potentially some callsites don't know how to handle those new error codes (from Approach B) correctly - which might introduce new bugs. I am rather for Approach A, but open to suggestions.

Sorry, I somehow missed comment 2. :( > I think a real parity implementation is not possible anymore due to the change of the security checks setup Note that I'm not requiring parity. I'm requiring an explicit analysis of the changes we're making in behavior and why we think they're OK to make. > but rather lets nsCorsListenerProxy perform that check (see attached patch). Does the nsCorsListenerProxy bit get propagated back out to nsContentSecurityManager? It's not obvious to me that it does.... Note also that it's not clear that we want to turn a CheckLoadURI failure for a _redirect_ into a SetBlockedRequest call, which is what doing things in nsContentSecurityManager will do. So maybe the right answer here is to simply not do a SetBlockedRequest call in the case when CheckLoadURI fails. Right now, a CheckLoadURI failure gets turned into REJECT_REQUEST, which is then mapped to the mBroken state. The only difference from what we have now would be what happens in terms of the ClearPendingRequest/ClearCurrentRequest calls, and that might be acceptable. We would need to test to see what the actual behavior change is, if any. > That reminds me that we probably also have to update the CORSListenerProxy to skip the > CheckLoadURIWithPrincipal check in case the load comes from a docshell of appType: APP_TYPE_EDITOR. I actually suspect that may not be needed: this would only matter for images being edited, with a "crossorigin" attribute, and linking to something like file://. In practice, the only use case I know of for the "images being edited and linking to file://" thing is Thunderbird HTML mail composition, and that's not going to be using the "crossorigin" attribute, I'd think. Anyway, I think I'm OK with approach (A); I just want us to have a clear list of behavior changes it introduces and then analysis of whether those behavior changes are OK.

Before commenting further, I'd like to understand what behavior we *currently* have in the following cases: 1) Server responds with something that isn't a valid image 2) CheckLoadURIWithPrincipal check failed 3) <img> uses CORS but server responds with the wrong CORS headers 4) CSP policy check failed Intuitively, I would have expected all of the above to have the same behavior. But maybe there are good reasons not to?

Try from ckerschb's patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89d78d551f3f Currently I am looking into the failure from parser/htmlparser/tests/mochitest/test_bug102699.html. So far the assert will be triggered when loading http://mochi.test:8888/tests/parser/htmlparser/tests/mochitest/file_bug102699.sjs, with the documentURI is http://mochi.test:8888/tests/parser/htmlparser/tests/mochitest/test_bug102699.html. Originally (without the patch applied) the uri (http://mochi.test:8888/tests/parser/htmlparser/tests/mochitest/file_bug102699.sjs) will be rejected when after calling nsContentUtils::CanLoadImage I'll continue to check this. (I change NS_ASSERT to MOZ_ASSERT in nsContentUtils::LoadImage() Assertion failure: loadGroup || IsFontTableURI(documentURI) (Could not get loadgroup; onload may fire too early), at /home/allstars/src/gecko-dev/dom/base/nsContentUtils.cpp:3206 #01: nsContentUtils::LoadImage(nsIURI*, nsINode*, nsIDocument*, nsIPrincipal*, nsIURI*, mozilla::net::ReferrerPolicy, imgINotificationObserver*, int, nsAString_internal const&, imgRequestProxy**, unsigned int) (/home/allstars/src/gecko-dev/dom/base/nsContentUtils.cpp:3205 (discriminator 7)) #02: nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsImageLoadingContent::ImageLoadType, nsIDocument*, unsigned int) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.cpp:890) #03: nsImageLoadingContent::LoadImage(nsAString_internal const&, bool, bool, nsImageLoadingContent::ImageLoadType) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.cpp:781) #04: mozilla::dom::HTMLImageElement::LoadSelectedImage(bool, bool, bool) (/home/allstars/src/gecko-dev/dom/html/HTMLImageElement.cpp:984 (discriminator 4)) #05: mozilla::dom::HTMLImageElement::MaybeLoadImage() (/home/allstars/src/gecko-dev/dom/html/HTMLImageElement.cpp:709)

Yoshi: This was listed as "domsecurity-active" but with no priority. Are you actively working on this one in the next quarter or should we assign it to someone else?

Yes it's in my TODO list, I was interrupted by the FirstPartyIsolation back in July and didn't finish this. Sorry about that. I will fix this in the next quarter. I am not sure the priority is, if you need that information I'll ask Christoph.

So far I removed the assert on the loadGroup mentioned in comment 6, the tests that failed in the beginning now are passed, however there some tests now will timeout, /service-workers/service-worker/fetch-csp.https.html, will continue to look into this.

Comment on attachment 8747705 [details] [diff] [review] bug_1267075_asyncopen2_imageloadingcontent.patch Review of attachment 8747705 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentPolicy.cpp @@ +149,5 @@ > + externalType == nsIContentPolicy::TYPE_IMAGESET) { > + nsCOMPtr<nsIImageLoadingContent> img = > + do_QueryInterface(requestingContext); > + if (img) { > + img->SetBlockedRequest(contentLocation, *decision); This line will cause the timeout in http://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/fetch-csp-iframe.html#49 neither onload nor onerror will be called. And the reason is when we call SetBlockedRequest here, then it goes to http://searchfox.org/mozilla-central/source/image/imgRequestProxy.cpp#339, it will set mCancel to true, later when OnLoadComplete is called, http://searchfox.org/mozilla-central/source/image/imgRequestProxy.cpp#826 the listener->Notify won't be called since mCancel is set to true before.

Comment on attachment 8828688 [details] [diff] [review] Part 1: call SetBlockedRequest when CSP check failed Review of attachment 8828688 [details] [diff] [review]: ----------------------------------------------------------------- Hi BZ This is mostly ckerschb's original patch. However in his original patch, I found that calling ClearPendingRequest/ClearCurrentRequest in SetBlockedRequest will cause timeout in tests like /service-workers/service-worker/fetch-csp.https.html, as I've mentioned in some details in Comment 10. And I also removed the aURI in that function, because if calling nsContentUtils::LoadImage failed, it will also replace the mCurrentURI in http://searchfox.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#929 Thanks

Comment on attachment 8828689 [details] [diff] [review] Part 2: Remove assert for loadGroup. Review of attachment 8828689 [details] [diff] [review]: ----------------------------------------------------------------- Hi BZ Originally those cases without LoadGroup would return in CanLoadImage. However now we removed calling CanLoadImage in LoadImage, for some tests it will trigger the assertion because of not having loadGroup, for example the test from http://searchfox.org/mozilla-central/source/layout/reftests/svg/image/imported-image-01.svg#7 I think that's because DOMParser didn't provide loadGroup in the first place. However from the stack trace, I am not sure how can I figure out this document is from DOMParser. #01: nsContentUtils::LoadImage(nsIURI*, nsINode*, nsIDocument*, nsIPrincipal*, nsIURI*, mozilla::net::ReferrerPolicy, imgINotificationObserver*, int, nsAString_internal const&, imgRequestProxy**, unsigned int) (/home/allstars/src/gecko-dev/dom/base/nsContentUtils.cpp:3305 (discriminator 7)) #02: nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsImageLoadingContent::ImageLoadType, bool, nsIDocument*, unsigned int) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.cpp:899) #03: nsImageLoadingContent::LoadImage(nsAString_internal const&, bool, bool, nsImageLoadingContent::ImageLoadType) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.cpp:774) #04: mozilla::dom::SVGImageElement::LoadSVGImage(bool, bool) (/home/allstars/src/gecko-dev/dom/svg/SVGImageElement.cpp:136) #05: mozilla::dom::SVGImageElement::AfterSetAttr(int, nsIAtom*, nsAttrValue const*, bool) (/home/allstars/src/gecko-dev/dom/svg/SVGImageElement.cpp:165) #06: mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) (/home/allstars/src/gecko-dev/dom/base/Element.cpp:2513) #07: mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) (/home/allstars/src/gecko-dev/dom/base/Element.cpp:2390) #08: nsXMLContentSink::AddAttributes(char16_t const**, nsIContent*) (/home/allstars/src/gecko-dev/dom/xml/nsXMLContentSink.cpp:1405) #09: nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, bool) (/home/allstars/src/gecko-dev/dom/xml/nsXMLContentSink.cpp:973) #10: nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int) (/home/allstars/src/gecko-dev/dom/xml/nsXMLContentSink.cpp:918) #11: nsExpatDriver::HandleStartElement(char16_t const*, char16_t const**) (/home/allstars/src/gecko-dev/parser/htmlparser/nsExpatDriver.cpp:382) #12: Driver_HandleStartElement(void*, char16_t const*, char16_t const**) (/home/allstars/src/gecko-dev/parser/htmlparser/nsExpatDriver.cpp:70) #13: doContent (/home/allstars/src/gecko-dev/parser/expat/lib/xmlparse.c:2469) #14: contentProcessor (/home/allstars/src/gecko-dev/parser/expat/lib/xmlparse.c:2098) #15: doProlog (/home/allstars/src/gecko-dev/parser/expat/lib/xmlparse.c:4078) #16: prologProcessor (/home/allstars/src/gecko-dev/parser/expat/lib/xmlparse.c:3812) #17: prologInitProcessor (/home/allstars/src/gecko-dev/parser/expat/lib/xmlparse.c:3630) #18: MOZ_XML_Parse (/home/allstars/src/gecko-dev/parser/expat/lib/xmlparse.c:1530) #19: nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*) (/home/allstars/src/gecko-dev/parser/htmlparser/nsExpatDriver.cpp:1016) #20: nsExpatDriver::ConsumeToken(nsScanner&, bool&) (/home/allstars/src/gecko-dev/parser/htmlparser/nsExpatDriver.cpp:1113) #21: nsParser::Tokenize(bool) (/home/allstars/src/gecko-dev/parser/htmlparser/nsParser.cpp:1945) #22: nsParser::ResumeParse(bool, bool, bool) (/home/allstars/src/gecko-dev/parser/htmlparser/nsParser.cpp:1462 (discriminator 1)) #23: nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (/home/allstars/src/gecko-dev/parser/htmlparser/nsParser.cpp:1842) #24: mozilla::dom::DOMParser::ParseFromStream(nsIInputStream*, char const*, int, char const*, nsIDOMDocument**) (/home/allstars/src/gecko-dev/dom/base/DOMParser.cpp:300) #25: mozilla::dom::DOMParser::ParseFromString(nsAString_internal const&, char const*, nsIDOMDocument**) (/home/allstars/src/gecko-dev/dom/base/DOMParser.cpp:128) #26: mozilla::dom::DOMParser::ParseFromString(nsAString_internal const&, mozilla::dom::SupportedType, mozilla::ErrorResult&) (/home/allstars/src/gecko-dev/dom/base/DOMParser.cpp:62) #27: mozilla::dom::DOMParserBinding::parseFromString(JSContext*, JS::Handle<JSObject*>, mozilla::dom::DOMParser*, JSJitMethodCallArgs const&) (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dom/bindings/DOMParserBinding.cpp:75) # And this assertion is added since the first release of Mozilla, and the imgLoader::LoadImage will check if aLoadGroup is provided or not. So I'd like to ask if we can remove the assertion, or you have some suggestions that we could know the document is from a DOMParser? Thanks

Comment on attachment 8828690 [details] [diff] [review] Part 3: cancel imgRequestProxy if asyncOpen2 failed. Review of attachment 8828690 [details] [diff] [review]: ----------------------------------------------------------------- Hi BZ This is a just one line patch, however this takes most of the time when I tried to fix this bug. :( This problem is if we found a cache hit, then we could go through ValidateRequestWithNewChannel to validate the cache. Then if the CSP check fail(asyncOpen2() will fail), then the imgRequestProxy will remain there, and cause the timeout. I run into problem when running mochitest browser/base/content/test/general/browser_aboutHome.js in non-e10s mode. In the beginning, browser.xul will load defaultFavicon.png, will create an image cache there. Next time when the test starts to run, when it loads about:home, then it will try to load defaultFavicon.png, it will found an image cache hit (loaded previously by browser.xul), and call ValidateRequestWithNewChannel there, however the asyncOpen2 call failed, and the imgRequestProxy is added to the loadGroup of about:home, and never be notified until timeout. Thanks

I'm not likely to get to this before Tuesday, sorry. Part 3 needs review from an imagelib peer. As far as comment 15 goes... it goes to the heart of what I feel is wrong about the new setup. The reason DOMParser has no loadgroup is that it makes no sense for it to have one, because it should NEVER DO ANY LOADS. Ever. We implemented this policy as a content policy, because content policies always got checked before we ever talked to necko. This is no longer the case, and arguably we should move the "is a data document" check out of content policy or something... In any case, instead of removing the assertion I believe we should do the check earlier so we don't waste tons of time for all the images in a data document calling into the image loader, etc, just to decide that oh, we don't want to do the load after all. That is, put an "is data document" check in nsContentUtils::LoadImage. Yes, it's a duplicated check. But unlike the monstrosity that is nsDataDocumentContentPolicy::ShouldLoad this would just be a "if (doc->IsLoadedAsData() && !doc->IsStaticDocument())" check, which is _really_ fast and should help prevent the horrible performance regression these patches would otherwise cause for image-heavy data documents.

Comment on attachment 8828690 [details] [diff] [review] Part 3: cancel imgRequestProxy if asyncOpen2 failed. Review of attachment 8828690 [details] [diff] [review]: ----------------------------------------------------------------- Hi Timothy Originally the content security check will be done in CanLoadImage, however now we plan to remove it now (Part 1). Then I found some problems in imgLoader, if the request is denied for security reason, the imgRequestProxy is not canceled, and caused timeout in some tests, please see comment 16 for details. Thanks

Comment on attachment 8828690 [details] [diff] [review] Part 3: cancel imgRequestProxy if asyncOpen2 failed. Seems like there's more cleanup that we need to do if AsyncOpen2 fails. From ValidateRequestWithNewChannel just before this line: request->SetValidator(hvc); Does the validator get cleared on request?

(In reply to Timothy Nikkel (:tnikkel) from comment #20) > Comment on attachment 8828690 [details] [diff] [review] > Part 3: cancel imgRequestProxy if asyncOpen2 failed. > > Seems like there's more cleanup that we need to do if AsyncOpen2 fails. From > ValidateRequestWithNewChannel just before this line: > > request->SetValidator(hvc); > > Does the validator get cleared on request? Yes, the validator will be cleared in the original code (even without my patch applied). from http://searchfox.org/mozilla-central/source/image/imgLoader.cpp#2718 Once the channel is destroyed the validated will get destroyed, too. I print out the stack below. #02: imgCacheValidator::~imgCacheValidator() (/home/allstars/src/gecko-dev/image/imgLoader.cpp:2724) #03: imgCacheValidator::Release() (/home/allstars/src/gecko-dev/image/imgLoader.cpp:2696 (discriminator 9)) #04: nsCOMPtr<nsIInterfaceRequestor>::~nsCOMPtr() (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:406) #05: nsBaseChannel::~nsBaseChannel() (/home/allstars/src/gecko-dev/netwerk/base/nsBaseChannel.cpp:68) #06: nsFileChannel::~nsFileChannel() (/home/allstars/src/gecko-dev/netwerk/protocol/file/nsFileChannel.cpp:293) #07: nsFileChannel::~nsFileChannel() (/home/allstars/src/gecko-dev/netwerk/protocol/file/nsFileChannel.cpp:293) #08: nsHashPropertyBag::Release() (/home/allstars/src/gecko-dev/xpcom/ds/nsHashPropertyBag.cpp:258 (discriminator 10)) #09: nsBaseChannel::Release() (/home/allstars/src/gecko-dev/netwerk/base/nsBaseChannel.cpp:328) #10: nsFileChannel::Release() (/home/allstars/src/gecko-dev/netwerk/protocol/file/nsFileChannel.cpp:435) #11: nsCOMPtr<nsIChannel>::~nsCOMPtr() (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:406) #12: imgLoader::ValidateRequestWithNewChannel(imgRequest*, nsIURI*, nsIURI*, nsIURI*, mozilla::net::ReferrerPolicy, nsILoadGroup*, imgINotificationObserver*, nsISupports*, unsigned int, unsigned int, imgRequestProxy**, nsIPrincipal*, int) (/home/allstars/src/gecko-dev/image/imgLoader.cpp:1636) #13: imgLoader::ValidateEntry(imgCacheEntry*, nsIURI*, nsIURI*, nsIURI*, mozilla::net::ReferrerPolicy, nsILoadGroup*, imgINotificationObserver*, nsISupports*, unsigned int, unsigned int, bool, imgRequestProxy**, nsIPrincipal*, int) (/home/allstars/src/gecko-dev/image/imgLoader.cpp:1841) #14: imgLoader::LoadImage(nsIURI*, nsIURI*, nsIURI*, mozilla::net::ReferrerPolicy, nsIPrincipal*, nsILoadGroup*, imgINotificationObserver*, nsINode*, nsIDocument*, unsigned int, nsISupports*, unsigned int, nsAString_internal const&, imgRequestProxy**) (/home/allstars/src/gecko-dev/image/imgLoader.cpp:2125) #15: nsContentUtils::LoadImage(nsIURI*, nsINode*, nsIDocument*, nsIPrincipal*, nsIURI*, mozilla::net::ReferrerPolicy, imgINotificationObserver*, int, nsAString_internal const&, imgRequestProxy**, unsigned int) (/home/allstars/src/gecko-dev/dom/base/nsContentUtils.cpp:3325) #16: nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsImageLoadingContent::ImageLoadType, bool, nsIDocument*, unsigned int) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.cpp:885) #17: nsImageLoadingContent::LoadImage(nsAString_internal const&, bool, bool, nsImageLoadingContent::ImageLoadType) (/home/allstars/src/gecko-dev/dom/base/nsImageLoadingContent.cpp:774) I think the problem is most related to loadGroup. The line before asyncOpen2, // Add the proxy without notifying hvc->AddProxy(req); will add the proxy into the loadGroup, however there seems no error handling if asyncOpen2 failed, so the proxy keeps remaining there.

check for data document as Bz suggested.

Comment on attachment 8828690 [details] [diff] [review] Part 3: cancel imgRequestProxy if asyncOpen2 failed. Okay thanks for clearing that up. Note that CancelAndForgetObserver does the remove from load group async, but that's okay because the load group keeps a reference to it.

I'm really sorry for the lag here. I'm not going to get to this until Monday.... :( Needs really careful review, unfortunately.

OK. So I finally managed to mostly page this back in. I'm not seeing anything addressing my concerns, as expressed in the quote in comment 2 or directly in comment 4. Those really do need addressing. There's no point asking me to keep reviewing the same patch that makes some set of behavior changes without figuring out what those behavior changes are and then deciding whether they're OK. But the first step is figuring out what they are!

Comment on attachment 8829370 [details] [diff] [review] Part 2: bait out early if it's for data document. s/bait/bail/ in the commit message. Also, looks like my comment misled you, and you didn't double-check :(. This behavior does NOT match nsDataDocumentContentPolicy::ShouldLoad. The check there is: if (doc->IsLoadedAsData()) { if (!doc->IsStaticDocument() || aContentType != nsIContentPolicy::TYPE_FONT) { *aDecision = nsIContentPolicy::REJECT_TYPE; OK, our aContentType is not nsIContentPolicy::TYPE_FONT. So that inner condition always tests true for images. So you just want to check IsLoadedAsdata().... r- for this part. Also, the old code used to call SetBlockedRequest, but we're no longer doing that. I _think_ that's probably OK, but you should probably try to verify that independently....

Comment on attachment 8828688 [details] [diff] [review] Part 1: call SetBlockedRequest when CSP check failed Needs an analysis, or at least a list of, of the behavior changes this produces.

Comment on attachment 8837475 [details] [diff] [review] Part 1: call SetBlockedRequest when CSP check failed Review of attachment 8837475 [details] [diff] [review]: ----------------------------------------------------------------- Hi Boris In the case of HaveSize(mCurrentRequest) == 1 old setup: it will call ClearPendingRequest(BLOCKED) (called by SetBlockedRequest) in the new setup: ClearPendingRequest(SRC_CHANGED) will be called by PreparePendingRequest(), which is called by PrepareNextRequest() note that the canceling reason is different here, one is BLOCKED and the other is SRC_CHANGED, there's a TODO here I'll explain in the end. and now mPendingRequestFlags will have IS_IMAGESET flag if it's for imageset. I didnt reset it for now, I'll try later. In the case of !HaveSize(mCurrentRequest) old setup: SetBlockedRequest will do the following: ClearPendingRequest(BLOCKED) ClearCurrentRequest(BLOCKED) mImageBlockingStatus will be set, mCurrentRequestFlags will keep the IS_IMAGESET flag if it has before mCurrentURI will be set to the URI of blocked image. new setup: ClearCurrentRequest(SRC_CHANGED) will be called by PrepareCurrentRequest() and inside PrepareCurrentRequest, mCurrentRequstFlags will have the IS_IMAGESET flag then when SetBlockedRequest is called, it will call ClearPendingRequest(BLOCKED); set the mImageBlockingStatus then mCurrentURI will be set to the URI of blocked image if calling nsContentUtils::LoadImage fails. http://searchfox.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#929 and the TODO here are in the case of HaveSize(mCurrentRequest) == 1, the old setup calls ClearPendingRequest with the status code BLOCKED, now the new setup, we will call ClearPendingRequest with the status code SRC_CHANGED, This is the abuse of some legacy behavior mentioned in http://searchfox.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#1228 I am worried this could break some legacy addon/website, but I am not totally sure. and so for the case of !HaveSize(mCurrentRequest), we used to CancelPendingRequest with BLOCKED, and CancelCurrentRequest with BLOCKED, now we do CancelCurrentRequest with SRC_CHANGED, and CancelPendingRequest with BLOCKED. This case still do the same CancelPendingRequest with BLOCKED, however now CancelCurrentRequest has different status code now, which I am also worried about. There's another change here, There are other places will call CSP, for example ServiceWorkerEvemt, in http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#254 now in the new setup, if it tried to load an image that will be blocked, we will call SetBlockedRequest, and in the case of !HaveSize(mCurrentRequest), then we will call ClearPendingRequest (for the legacy behavior), and set the mImageBlockingStatus code. which the old setup doesn't do this at all.

Thank you for the updates! I'm not likely to get to this before Friday morning, unfortunately.... I'll see if I can manage before then.

Clear mPendingRequestFlags if the image is blocked, this is to make it have the same bahavior with the original setup.

I'm really sorry for the horrible lag here. :( This is the first thing on my list for Monday...

Comment on attachment 8837476 [details] [diff] [review] Part 2: bail out early if it's for data document. v2 r=me

Comment on attachment 8837923 [details] [diff] [review] Part 1: call SetBlockedRequest when CSP check failed I'm really sorry for the incredible delay here... Thank you again for writing down the differences in behavior. Have you actually done any testing of the IMAGE_BLOCKED vs SRC_CHANGED behavior and the showing of the broken image icons in the new setup? >+ // We only want to cancel the existing current request >+ // if size is not available. bz says the web depends on this behavior. Well, and it's in the HTML spec too, I'm pretty sure... >+ // TODO: Is there a bug tracking this? >+ // PreparePendigRequest will update mPendingRequestFlags, now since the "PreparePendingRequest". I don't understand this comment. This code would run _after_ PreparePendingRequest, no? So in what sense is it that "PreparePendingRequest will ..."? Is this trying to say that PreparePendingRequest set the mPendingRequestFlags (in the past, not the future), and now that we've decided to block it we should reset them back to 0? Can we assert in this branch that mPendingRequest is null? Seems like we should be able to, right? Except for that service worker business, maybe, where we can get calls to SetBlockedRequest are arbitrary points later during the load. But I don't see how that case can work correctly at all: it might be reporting a request being blocked that doesn't even have anything to do with the image state anymore! I think it's pretty important that we NOT end up doing any work in SetBlockedRequest except in the case when it's happening sync from our actual image load attempt. If we want to do something for the service worker case, we need to sit down and very carefully redesign all this stuff so that the SetBlockedRequest call clearly communicates _which_ request is blocked or something. >+ ClearPendingRequest(NS_ERROR_IMAGE_BLOCKED, > Some(OnNonvisible::DISCARD_IMAGES)); This call doesn't make sense to me. If we're in this branch, that means !HaveSize(mCurrentRequest). But then in the normal image load case that means that we did the load on mCurrentRequest anyway. In other words, mPendingRequest should be null here, and the ClearPendingRequest call is a confusing no-op. >@@ -626,17 +627,31 @@ nsContentSecurityManager::CheckChannel(nsIChannel* aChannel) When does this run? Why do we need this in addition to the nsContentPolicy::CheckPolicy checks? >@@ -915,23 +917,51 @@ nsCORSListenerProxy::UpdateChannel(nsIChannel* aChannel, This _definitely_ runs async (e.g. on redirects) and will completely confuse the logic we have, which assumes that the call happens sync under LoadImage. Like the worker case, please don't do this except maybe in a followup that completely redesigns how this works. We really need to just write tests that exercise the edge cases here, especially the edge cases in which the behavior differs from what it used to be, and see what actually happens... I don't think anyone understands this code well enough at this point to model that sort of thing in their head. :(

Oh, and I'm pretty convinced that the old "keep the old request imageset flag" behavior is nuts and we should just set the new request imageset flag like your patch does. Apart from that and the possible async calls, I _think_ I'm also convinced that the SRC_CHANGED vs IMAGE_BLOCKED thing is the main difference. We should see what that actually does.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #35) > I'm really sorry for the incredible delay here... Thank you again for > writing down the differences in behavior. Thanks for the review, I know you've always been crazy busy, so no worries for the delay. :) > > Have you actually done any testing of the IMAGE_BLOCKED vs SRC_CHANGED > behavior and the showing of the broken image icons in the new setup? > I only ran try server, and it is all green. As you said, the main difference I made is the SRC_CHANGED vs IMAGE_BLOCKED thing, however I am not sure if it's okay so I mark them as TODO. > >+ // We only want to cancel the existing current request > >+ // if size is not available. bz says the web depends on this behavior. > > Well, and it's in the HTML spec too, I'm pretty sure... > This comment is copied from the original SetBlockedRequest. > >+ // TODO: > > Is there a bug tracking this? > I am not sure if bug 583491 is what you meant here, it's the follow up bug from bug 521497. (also reviewed by you) > >+ // PreparePendigRequest will update mPendingRequestFlags, now since the > > "PreparePendingRequest". > > I don't understand this comment. This code would run _after_ > PreparePendingRequest, no? So in what sense is it that > "PreparePendingRequest will ..."? > Yes, SetBlockedRequest will run _after_ PreparePendingRequest. Sorry for my poor English. :( > Is this trying to say that PreparePendingRequest set the > mPendingRequestFlags (in the past, not the future), and now that we've > decided to block it we should reset them back to 0? Yes > Can we assert in this branch that mPendingRequest is null? Seems like we > should be able to, right? Except for that service worker business, maybe, > where we can get calls to SetBlockedRequest are arbitrary points later > during the load. Yes, I'll add an assert. But for your information, there is already an assertion for the request should be null in http://searchfox.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#925 > I think it's pretty important that we NOT end up doing any work in > SetBlockedRequest except in the case when it's happening sync from our > actual image load attempt. If we want to do something for the service > worker case, we need to sit down and very carefully redesign all this stuff > so that the SetBlockedRequest call clearly communicates _which_ request is > blocked or something. > Agree, and as I mentioned in my previous comment 14, I did get some problems in service-worker part from the first version of the patch. Right now the change passes webplatform tests, however if there is a more complicated use case of service worker, then we do need to revise SetBlockedRequest here. > >+ ClearPendingRequest(NS_ERROR_IMAGE_BLOCKED, > > Some(OnNonvisible::DISCARD_IMAGES)); > > This call doesn't make sense to me. If we're in this branch, that means > !HaveSize(mCurrentRequest). But then in the normal image load case that > means that we did the load on mCurrentRequest anyway. In other words, > mPendingRequest should be null here, and the ClearPendingRequest call is a > confusing no-op. > Yes, it's a little confusing here. But the calling ClearPendingRqeuest here is intentional, and it is to make sure to have the same behavior with the old setup. In the old setup, when the image is blocked in the !HaveSize(mCurrentRequest) case, the old setup will call ClearPendingRequest(IMAGE_BLOCKED), then ClearCurrentRequest(IMAGE_BLOCKED) (actually in the old setup, ClearPendingRequest will always be called regardless of !HaveSize(mCurrentRequest)) However in the new setup, PrepareNextRequest will only call ClearCurrentRequest, we need to call ClearPendingRequest. So I move the ClearPendingRequest in !HaveSize(mCurrentRequest) branch. As ClearPendingRequest is the 'illogical stuff' mentioned by bholley so I still leave it there. > >@@ -626,17 +627,31 @@ nsContentSecurityManager::CheckChannel(nsIChannel* aChannel) > > When does this run? Why do we need this in addition to the > nsContentPolicy::CheckPolicy checks? > In the old setup, if DoCheckLoadURIChecks failed, it will bail out http://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#634 then exit doContentSecurityChecks in http://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#475 hence the code won't call DoContentSecurityChecks. So in this case, (DoCheckLoadURIChecks failed), the code in nsContentPolicy::CheckPolicy won't be called. And if we don't call SetBlockedRequest here in nsContentSecurityManager::CheckChannel, then loading some images won't have the blockingStatus set. For example, loading defaultFavicon.png in about:home will fail in DoCheckLoadURIChecks. (If I remembered correctly..) > >@@ -915,23 +917,51 @@ nsCORSListenerProxy::UpdateChannel(nsIChannel* aChannel, > > This _definitely_ runs async (e.g. on redirects) and will completely confuse > the logic we have, which assumes that the call happens sync under LoadImage. > Like the worker case, please don't do this except maybe in a followup that > completely redesigns how this works. > Okay will do. > We really need to just write tests that exercise the edge cases here, > especially the edge cases in which the behavior differs from what it used to > be, and see what actually happens... I > I _think_ I'm also convinced that the SRC_CHANGED vs IMAGE_BLOCKED thing is > the main difference. We should see what that actually does. Totally agree on this part. The two things (marked as TODO in my patch) I've changed are: 1. In the HaveSize(mCurrentRequest) case, We used to do ClearPendingRequst(BLOCKED), now we change it ClearPendingRequest(SRC_CHANGED); This breaks the 'illogical stuff' as mentioned by bholley's original patch. 2, When !HaveSize(mCurrentRequest), we used to ClearCurrentRequest(BLOCKED), now it's ClearCurrentRequest(SRC_CHANGED) But do you have more information for (1), the 'illogical stuff'? Why should we always call ClearPendingRequest in the old setup? Since you reviewed that part before, can you give me more background on this? Meanwhile I'll check the spec for image loading for (2). Thanks again for your prompt review.

> I only ran try server, and it is all green. I would be moderately shocked if anything on try exercises this codepath at all. :( For one thing, until recently (with CSP) that wasn't very simple to do. > This comment is copied from the original SetBlockedRequest. Yes, I did notice that. Just trying to correct the record! > I am not sure if bug 583491 is what you meant here, That seems plausible, yes. Change the comment to reference that bug number, maybe? > But for your information, there is already an assertion for the request should > be null in > http://searchfox.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#925 That's not quite the same thing. That's asserting that it's null after we return from a failed call to imagelib. So _if_ all the calls to SetBlockedRequest happen before that return, we're good. The assert I'm suggesting would catch calls to SetBlockedRequest that happen _after_ the return... Maybe what we should do is simply set a boolean member while calling into imagelib to indicate that we're in that call. When that boolean is not set, ignore calls to SetBlockedRequest. And when that boolean _is_ set, ignore or throw for calls to LoadImage, since it means some extension is being evil and reentering us. Then file a followup for how we want to handle async stuff like service workers. > actually in the old setup, ClearPendingRequest will always be called > regardless of !HaveSize(mCurrentRequest) Right. But in the old setup if !HaveSize(mCurrentRequest) it will be a no-op, no? It was sort of written that way for simplicity, since we should never have a pending request after we got blocked... Specifically, if !HaveSize(mCurrentRequest), then either it's null altogether or it has no size yet. In the latter case we would not have a pending request, because pending requests only get started when we have an mCurrentRequest which has size. In the former case, it could be that we had both requests and then someone nulled out only mCurrentRequest.... but no one is supposed to do that. And I don't think anyone does. I'm pretty sure we can just assert !mPendingRequest in the !HaveSize(mCurrentRequest) branch. > So in this case, (DoCheckLoadURIChecks failed), the code in nsContentPolicy::CheckPolicy won't be called. Ah, right. Is this CheckChannel thing only called for the original load or also for redirects? As in, can it trigger async calls back into nsImageLoadingContent? > But do you have more information for (1), the 'illogical stuff'? I _think_ the "illogical stuff" bholley's taking about is this case: 1) Create a new <img>. 2) Start a load. There is now a current request but no pending request. 3) Try to start another load before the first one has a size available. 4) The second load gets blocked. In this situation, the code before and after bholley's change would cancel the load started in step _2_ with the NS_ERROR_IMAGE_BLOCKED that corresponds to the load from step _3_. That's the "illogical" bit. I don't recall this stuff very well. It's been a few years... > Why should we always call ClearPendingRequest in the old setup? The code before bholley's change looked like this: if (mPendingRequest) { // cancel it } if (mCurrentRequest) { // Cancel it if it does not have a known size, or if some other conditions hold. } or so. The "cancel the pending request" bit _could_ have been moved into the "has a known size" branch but it would have been more confusing to write it that way. bholley more or less preserved that logic, and that's what you see in SetBlockedRequest() right now. At this point the "some other conditions" bit is gone, so it would be pretty simple to only ClearPendingRequest in the HaveSize(mCurrentRequest) case. We just never rearranged the code that way.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #35) > We really need to just write tests that exercise the edge cases here, > especially the edge cases in which the behavior differs from what it used to > be, and see what actually happens... I don't think anyone understands this > code well enough at this point to model that sort of thing in their head. :( (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #38) >> So in this case, (DoCheckLoadURIChecks failed), the code in nsContentPolicy::CheckPolicy won't be called. > > Ah, right. Is this CheckChannel thing only called for the original load or > also for redirects? As in, can it trigger async calls back into > nsImageLoadingContent? > So far I am still trying to address bz's these two comments 1. write test case. 2. make sure CheckChannel doesn't trigger async calls.

Comment on attachment 8855192 [details] [diff] [review] Part 1: call SetBlockedRequest when CSP check failed v2 Review of attachment 8855192 [details] [diff] [review]: ----------------------------------------------------------------- Hi BZ (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #35) > Have you actually done any testing of the IMAGE_BLOCKED vs SRC_CHANGED > behavior and the showing of the broken image icons in the new setup? > In the new setup the 'broken' image icon won't show up if the original image is blocked, but neither does the old setup. Do we have to show the broken icon? I am not sure the correct behavior here. (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #38) >> So in this case, (DoCheckLoadURIChecks failed), the code in nsContentPolicy::CheckPolicy won't be called. > > Ah, right. Is this CheckChannel thing only called for the original load or > also for redirects? As in, can it trigger async calls back into > nsImageLoadingContent? > CheckChannel will also be called from AsyncOnChannelRedirect http://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#490 Check the code there it seems it won't trigger any async calls back to nsImageLoadingContent. But I am not sure how to write some code to make sure about this, Can you give me some suggestions here? Thanks

Comment on attachment 8855194 [details] [diff] [review] Part 4: add a boolean to prevent calling from ServiceWorker Review of attachment 8855194 [details] [diff] [review]: ----------------------------------------------------------------- Hi BZ This is base on your comment 38 <quote> "Maybe what we should do is simply set a boolean member while calling into imagelib to indicate that we're in that call. When that boolean is not set, ignore calls to SetBlockedRequest. And when that boolean _is_ set, ignore or throw for calls to LoadImage, since it means some extension is being evil and reentering us. Then file a followup for how we want to handle async stuff like service workers." </quote>

Comment on attachment 8855195 [details] [diff] [review] Part 5: test for blocking image. Review of attachment 8855195 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #35) > Have you actually done any testing of the IMAGE_BLOCKED vs SRC_CHANGED behavior Unfortunately I don't know a good way to test that. The status code is passed to imgRequest. However because the load is blocked, so I cannot get any request from nsIImageLoadingContent, But I have one test to test blocking the pending request while the current request is still loading. Can you help to check this? Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #48) > In the new setup the 'broken' image icon won't show up if the original image > is blocked, but neither does the old setup. > Do we have to show the broken icon? I am not sure the correct behavior here. > I guess the broken image is shown from nsImageFrame http://searchfox.org/mozilla-central/source/layout/generic/nsImageFrame.cpp#1400 but it depends on DisplayAltFeedback to be called. In old setup, for example, image blocked by CSP, the function DisplayAltFeedback is not called, so the broken icon is not shown.

bz, review ping? Or is there anything I can do to help for the review? I noticed there's another [qf:p1] bug, bug 1347376, from the two profiles in https://bugzilla.mozilla.org/show_bug.cgi?id=1347376#c0, I noticed that the call NS_CheckContentLoadPolicy takes 7.1% ~ 9.6% of the entire loading time, right now we've done NS_CheckContentLoadPolicy twice, one in CanLoadImage, and the other in AsyncOpen2, so if we can land this bug, we can also help bug 1347376 to reduce the loading time. Or if you're too busy to review it, should I find other reviewers? Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #53) > we can also help bug 1347376 to reduce the loading time. > I mean help the image loading time, as bug 1347376 measured time in imgLoader, it didn't include the time spent in dom.

Sorry, I should get to this in the next few days. I've been swamped with security bugs, qf work, and stylo, but if this is showing up in qf profiles I should bump the priority... I just need to take the several hours it will take to page all this back in. :(

review ping? Sorry Bz I know you have been busy and just had a work week last week, but the r? has been almost two months, if you're still busy with other bugs, can I find another reviewer to do this? or perhaps I just need your 5 mins to check if my test cases could verify the image blocking code, that could also help. Thanks

Comment on attachment 8855195 [details] [diff] [review] Part 5: test for blocking image. >+ img.QueryInterface(Ci.nsIImageLoadingContent); This line is completely unnecessary. Please take it out. This applies to all the tests. >+ Assert.equal(img.imageBlockingStatus, Ci.nsIContentPolicy.ACCEPT); This assumes that the content policy decision is made synchronously under the appendChild call. Given that this is something we might actually want to change longer-term, I'd rather we didn't assume that, if possible. But if we really do need to check the state right after insertion, not at image load end, we may not have better options. Also, what, if anything, ensures the test doesn't finish before the image load completes and hence we get the expected load event (and don't get the unexpected error event)? It might also be worth using .matches() to check that the images match the right state pseudo-classes, in addition to explicitly checking the imageBlockingStatus. >+ Assert.equal(img.getRequest(Ci.nsIImageLoadingContent.CURRENT_REQUEST), req, >+ "CURRENT_REQUEST shouldn't be replaced."); Fix the indent, please? I'm still trying to page this stuff in, so not sure whether this tests all the bits we needed tested... :(

Comment on attachment 8855225 [details] [diff] [review] Part 4: add a boolean to prevent calling from ServiceWorker > Subject: Bug 1267075 - Part 4: add a boolean to prevent calling from ServiceWorker It's not just ServiceWorker. It's any async calls to SetBlockedRequest, right? Please adjust the commit message accordingly to describe what problem this is actually trying to solve, and point to the followup bug for coming up with a better solution (e.g. bug 1353685, but we might want a separate bug on the redirect thing) > mIsCallingFromLoadImage Perhaps mIsStartingImageLoad? >+ mIsCallingFromLoadImage = true; Probably better to use AutoRestore and scoping. Then you don't have to worry about early returns either. >+ NS_ENSURE_TRUE(mIsCallingFromLoadImage, NS_OK); This will warn, but this situation is normal and doesn't seem like it merits a warning. Please use a non-warning construct here. >+ // If this is false, it meas this call is from other places like s/meas/means/ r=me with the above fixed.

OK, so as I page more of this back in... Did the last paragraph of comment 35 ever get addressed? That is, creating tests that exercise the edge cases here? As far as I can tell, that has not happened. These don't have to be automated tests, as a first cut (and in fact might not be automated ever, if they involve extension content policies). But need to have a clear understanding of what behavior changes we're actually introducing, in what circumstances, and whether we're OK with those behavior changes. Right now I do not have this clear understanding. Do you? As I said above, I've tried wetware-modeling this, but I'm not succeeding at it. Is the idea to put all this off to bug 583491 or something? I'd really rather we actually understood _what_ we're putting off. [In reply to comment 48] > CheckChannel will also be called from AsyncOnChannelRedirect That would definitely call back into the image loading content async, right? [In reply to comment 50] > But I have one test to test blocking the pending request while the current request is still loading. OK, but what is that test really testing? That we've switched to the "we are blocked" state? Or something else? Sorry, I've really lost track of what behavior changes we're making there and what we're trying to test. :( [In reply to comment 52] > but it depends on DisplayAltFeedback to be called. Right, and this should be getting called any time we've got a broken image and are using an imageframe, I would think.

Comment on attachment 8855192 [details] [diff] [review] Part 1: call SetBlockedRequest when CSP check failed v2 I think the following part of comment 35 is still not quite addressed: I asked for an assertion that !mPendingRequest in the "HaveSize(mCurrentRequest)" case in SetBlockedRequest. It looks like this was added in the !HaveSize branch, but I am pretty sure I was talking about the HaveSize() case. It looks like we should just hoist the assert out of the if/else there altogether, right? Looks reasonable with that, modulo the above comments. I'm really sorry for the lag.... As expected, it took nearly a full workday to page in enough of this stuff to comment even somewhat intelligently, so I needed to find a workday with no meetings scheduled. :( I'm really hoping we can do a faster turnaround next time so this stuff is still sort of fresh in my mind, but I realize that at this point _you_ have to page it in. :(

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #60) > Comment on attachment 8855192 [details] [diff] [review] > Part 1: call SetBlockedRequest when CSP check failed v2 > > I think the following part of comment 35 is still not quite addressed: I > asked for an assertion that !mPendingRequest in the > "HaveSize(mCurrentRequest)" case in SetBlockedRequest. It looks like this > was added in the !HaveSize branch, but I am pretty sure I was talking about > the HaveSize() case. > Yeah, I recall now why I didn't add the assert, later in your comment 38, From comment 38 " > But for your information, there is already an assertion for the request should > be null in > http://searchfox.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#925 That's not quite the same thing. That's asserting that it's null after we return from a failed call to imagelib. So _if_ all the calls to SetBlockedRequest happen before that return, we're good. The assert I'm suggesting would catch calls to SetBlockedRequest that happen _after_ the return... Maybe what we should do is simply set a boolean member while calling into imagelib to indicate that we're in that call. .... " I thought in the end you suggested I could do the 'boolean trick' without the Assertion(!mPendingRequest); I think I misunderstood you at that moment... > It looks like we should just hoist the assert out of the if/else there > altogether, right? > Yes, I'll move the assert out of the if/else.

- removed img.QueryInterface(Ci.nsIImageLoadingContent) - Use await/async instead - wait until load/error event is received before finishing the test, and test the imageBlockingStatus after the event. However I am not sure how to test the pseudo class '-moz-suppressed', that class is only used in chrome, how can I test this in a mochitest-browser test ? Or should I write another chrome test for this? Bz, can you give me some suggestions? Thanks

Oh, that one is restricted, right. You should be able to do this in a mochitest-chrome test. Doing it in mochitest-browser is probably possible, but mochitest-chrome should be simpler.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #63) > Oh, that one is restricted, right. > > You should be able to do this in a mochitest-chrome test. Doing it in > mochitest-browser is probably possible, but mochitest-chrome should be > simpler. Hi BZ Could you give me more detail for how to test this in chrome? 1. I don't know how to block image in chrome code, as it's System Principal, therefore all loads should be granted, right? 2. I've checked nsCSSPaser.cpp, to make element.matches(":-moz-suppressed") return true, mIsChrome needs to be true, but it's true only in CSSParserImpl::ParseSheet, I don't know how to continue then....

> therefore all loads should be granted, right? I think you're have to do something like this: 1) Add a UA-level sheet (which also allows :-moz-suppressed and company) with some styling for those pseudo-classes. 2) Load an unprivileged document over http in a subframe which has images blocked and whatnot. 3) Use computed style to see which of the pseudo-classes ended up applying.

renaming test.

Thanks for bz and heycam's help for the test, I'll try to add more tests for other pseudo classes.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #65) > 2) Load an unprivileged document over http in a subframe which has images > blocked and whatnot. Bz, would like to confirm with you, now it's a content iframe, then matches("-moz-suppressed") will always throw, right? Given it isn't a chrome document now. Thanks

Right. That's why you'd have to have a UA sheet which uses those selectors and applies styles, and then look at the resulting computed style to see whether the selectors matched.

use imageCache to clear cache, otherwise it will have failure in non-e10s mode.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #59) > OK, so as I page more of this back in... > > Did the last paragraph of comment 35 ever get addressed? That is, creating > tests that exercise the edge cases here? As far as I can tell, that has not > happened. > The main difference in my patch, as you pointed out in comment 36, is the we used to post IMAGE_BLOCKED, now we post SRC_CHANGED However later I found the status is ignored, and imgRequest posts NS_BINDING_ABORTED in case of failure. http://searchfox.org/mozilla-central/source/image/imgRequest.cpp#284 Also as mentioned in Comment 50 that if the request is blocked, then img.getRequest will return null, so we cannot get the status code from the imgRequest itself, unless we need to modify imgRequest. So back to your question, I have some simple test in my Part 5 patch, See the last two tests, load_https_and_http_test and block_pending_request_test load_https_and_http_test tries to test the 'illogical stuff' you mentioned in comment 38 block_pending_request_test tests the case that after size info is available we change to another src. But currently the tests cannot test the behavior change (BLOCKED->SRC_CHANGED), unless we change the imgRequest code. > These don't have to be automated tests, as a first cut (and in fact might > not be automated ever, if they involve extension content policies). But > need to have a clear understanding of what behavior changes we're actually > introducing, in what circumstances, and whether we're OK with those behavior > changes. Right now I do not have this clear understanding. Do you? As I > said above, I've tried wetware-modeling this, but I'm not succeeding at it. > So the behavior change is IMG_BLOCKED->SRC_CHANGED, as explained above. In case of HaveSize(mCurrentRequest) is 0 1. img.src = "good.png"; 2. before the size information is available, we do img.src = "bad.png"; OLD setup: bad.png will be blocked in CanLoadImage 2.1 we fire error and loadend event 2.2 we call SetBlockedRequest with the URI is "bad.png" 3. In SetBlockedRequest, in this case HaveSize(mCurrentRequest) is 0, so we will post IMG_BLOCKED to mCurrentRequest, and save the imageBlockingStatus code. (ClearPendingRequest does nothing because at this moment there's no mPendingRequest.) NEW setup: 2.1 In PrepareNextRequest, because HaveSize(mCurrentRequest) is 0, we will call ClearCurrentRequest, this will post SRC_CHANGED to mCurrentRequest. 2.2 In SetBlockedRequest, we just save the imageBlockingStatue code. In case of HaveSize(mCurrentRequest) is 1 1. img.src = "good.png"; 2. size of good.png is known. 3. img.src = "bad.png"; OLD setup: 3.1. fire error and loadend 3.2. no-op, ClearPendingRequest does nothing because at this moment there's no mPendingRequest. NEW setup: 3.1 PrepareNextRequest->ClearPendingRequest, no-op 3.2 we just reset mPendingRequest flags > Is the idea to put all this off to bug 583491 or something? I'd really > rather we actually understood _what_ we're putting off. Yes, also if you think I need to change imgRequest so we can tell the status, let me know. > > [In reply to comment 50] > > But I have one test to test blocking the pending request while the current request is still loading. > > OK, but what is that test really testing? That we've switched to the "we > are blocked" state? Or something else? Sorry, I've really lost track of > what behavior changes we're making there and what we're trying to test. :( > As explained above, I test the two behaviors when HaveSize(mCurrentRequest) is true/false. > [In reply to comment 52] > > but it depends on DisplayAltFeedback to be called. > > Right, and this should be getting called any time we've got a broken image > and are using an imageframe, I would think. Yes, see the new added Part 6 test, I could see the 'broken' image shown for the -moz-broken and -moz-user-disabled.

- Moved MOZ_ASSERT(!mPendingRequest, "mPendingRequest should be null."); out of if/else block within SetBlockedRequest - Removed calling SetBlockedRequest in nsContentSecurityManager::CheckChannel and file a seperate bug.

Hi BZ Thanks for the review, sorry it took me a while to write the state pseudo class test, please also see my comment 72 for your previous questions. I'd like to requst for your review for Part 1 and Part 6, can you help to review again? Thanks

> However later I found the status is ignored, and imgRequest posts NS_BINDING_ABORTED in case of failure. Right, but the question was whether either of the other two values is checked for by anything else. I just looked carefully, and it looks to me like they aren't used at all. If so, can we just remove them both and switch directly to using NS_BINDING_ABORTED here too? > But currently the tests cannot test the behavior change (BLOCKED->SRC_CHANGED) Right, if it's a behavior change that affects nothing, then we just don't care. Thank you for explaining what's going on!

Comment on attachment 8875215 [details] [diff] [review] Part 6: test state pseudo class for image blocking. >+ img2.src = "https://no_such_url.png"; I'm not sure what this will actually do when run on infra (e.g. might try resolving and connecting and then crashing). Maybe use "https://test.invalid", which is known to be invalid and ok to use? >+ img3.src = "https://no_such_url.png"; Same here, though maybe you should use a valid url too, just to make sure it still gets blocked. r=me for the rest; this is great.

Comment on attachment 8875671 [details] [diff] [review] Part 1: call SetBlockedRequest when CSP check failed. v3 r=me. Thank you again for bearing with me on this one; I know I've been very laggy. :(

use NS_BINDING_ABORTED instead, > If so, can we just remove them both and switch directly to using NS_BINDING_ABORTED here too? removing the error code also needs servo change, so I filed bug 1371545.

updated the test url for img2 and img3.

Boris, thanks for your help and review :)

My try is green, https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1c94ca2edd4d67d46398159ac7ad96c3f4108ee However right now we're about to branch, so I'll push them next Monday.

In comment 53 I mentioned about this can help our image loading. Just did a profiling on today's fresh nightly build, commit 72ecaae42e7964618bbbfe38be6b971af2657e8b Merge: 8376e0e26174 56f2c75537da Author: Ryan VanderMeulen <ryanvm@gmail.com> Date: Sun Jun 11 22:06:40 2017 -0400 Merge inbound to m-c. a=merge profiling result is here https://perfht.ml/2rm453h I took a screen shot in https://screenshots.firefox.com/Ig8NqLPEUKu0ZO2t/perf-html.io We can see from the call tree we spend 20% time on calling nsContentUtils::CanLoadImage Use anotehr fresh user profile to test again another profiling result: https://perfht.ml/2rQRzfw screen shot: https://screenshots.firefox.com/BQCBvyqNe2KIOC5x/perf-html.io This one spends 30% time on calling nsContentUtils::CanLoadImage So I think this bug could help us on reducing 20% ~ 30% image loading time. I also applied my patches and run a profiling, see https://perfht.ml/2rRbVFx we can see that we don't call CanLoadImage anymore.

BTW, forgot to mention the testing site is the one used in bug 1347376, it's https://jakearchibald.github.io/service-worker-benchmark/

Base on my profiling result in comment 82, by removing CanLoadImage in my Part 1 patch could help reduce 20%~30% image loading time, adding this as [qf].

Pushed by yhuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8feacdbd43fa Part 1: call SetBlockedRequest when CSP check failed. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/cc82ff5ef4fc Part 2: bail out early if it's for data document. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/5160225f68b8 Part 3: cancel imgRequestProxy if asyncOpen2 failed. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/721409fb579b Part 4: add a boolean to prevent calling asynchronously. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/808507feb098 Part 5: test for blocking image. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ac004e0ea9 Part 6: test state pseudo class for image blocking. r=bz

https://hg.mozilla.org/mozilla-central/rev/8feacdbd43fa https://hg.mozilla.org/mozilla-central/rev/cc82ff5ef4fc https://hg.mozilla.org/mozilla-central/rev/5160225f68b8 https://hg.mozilla.org/mozilla-central/rev/721409fb579b https://hg.mozilla.org/mozilla-central/rev/808507feb098 https://hg.mozilla.org/mozilla-central/rev/a1ac004e0ea9

Great job, Yoshi! Thanks for your patience and hard work to get this done. This is significant! Also thanks to Christoph, Bz, and all the reviewers.