Currently, when the security code decides whether the lock icon should be shown as secure, mixed, or insecure, the transfer mode of images gets ignored. Whether displayed images have been loaded over https or http, this does not influence the state of the lock icon.

*** Bug 181072 has been marked as a duplicate of this bug. ***

*** Bug 178591 has been marked as a duplicate of this bug. ***

*** Bug 169289 has been marked as a duplicate of this bug. ***

*** Bug 234663 has been marked as a duplicate of this bug. ***

*** Bug 219502 has been marked as a duplicate of this bug. ***

*** Bug 301275 has been marked as a duplicate of this bug. ***

is this being worked on? if not, who should be working on it? (or should this be deferred?) /cb

should be looked at. not going to make 1.8b4. deferring. /cb

Does this bug apply to all image loads or only ones that are "background" loaded? i.e., loads that use nsIRequest::BACKGROUND_LOAD so that they do not block "onload". Such loads do not generate nsIWebProgressListener events, which would explain why the secure browser UI would not get to observe them. For non-background image loads, it may be the case that the secure browser UI does not know how to deal with a nsIRequest that is a imgIRequest (and not a nsIChannel). So, two things probably: (1) WPL is not a sufficient observer API, and (2) imgIRequest does not expose nsIChannel::securityInfo.

This bug applies at least to images loaded using <img src=""> tags. I haven't yet looked into the background loading you mention. You are right wrt imgIRequest. I talked to Stuart today. I learned that the nsIRequest objects sent with the listener events are imgRequestProxy objects that do not implement nsIChannel. The associated channel must be obtained indirectly. However, Stuart does not want us to allow obtaining the channel from the proxy, but we agreed on a new interface that allows to obtain the associated security info, only. I'm attaching a very experimental initial patch, that I played with today. There are a couple of problems with the patch. It seems, we don't seem to get "data is tranfering" events on image requests. So maybe we should always assume that data has been transfered. Not sure if that's the right thing to do. It seems we have to filter out resource:// image requests, I hope that's fine because those are non-network requests. The img requests tries to release its pointer to the associated channel as soon as possible. For initial testing I changed that to keep it. Maybe we should store a pointer to the security info at the time the channel is released. While the patch detects the mixed mode situation, the mixed mode does not get set when using the back/forward buttons. I'm primarily attaching the patch to make sure the code won't get lost until I have time to work on it more.

blocking: integrity of the lock icon is important.

Do Web pages depend on this? Is this bug common in other browsers? If so, should we consider fixing this only for a new higher-security lock icon rather than the current one?

(In reply to comment #13) > Is this bug common in other browsers? No. Other browser like IE and Netscape 4.x do it right. Insecure content is either shown or hidden. Fixing this should be done together with bug 62178, so that loading of insecure content (images) can actually be blocked. > If so, > should we consider fixing this only for a new higher-security lock icon rather > than the current one? IMHO Mozilla's usual lock icon should be fixed. IMHO by default, on a secure page, we should not display any content loaded over insecure URLs, notify in the chrome that insecure content is not being shown, and allow to use that chrome to explicitly display it.

Kai, Should I re-assign this bug to you ?.

Shaver asked this bug should block 1.8.1. Dan, you had assigned this bug to yourself about a year ago. Do you intend to work on it in time? Some ideas can be found in the attached patch and in comment 10.

Not going to block FF2 for this. We need a well tested trunk patch first. If a low-risk patch appears (seems unlikely), then we'll consider it for FF2 beta2.

*** Bug 364482 has been marked as a duplicate of this bug. ***

Kai: have you (or anyone else) played with the year-old experimental code attached to this bug?

This patch is fully inspired by Kaie's patch. It stores securityInfo of the underlying data channel to each image proxy request and makes it accessible with new interface nsISecurityInfoProvider. There are following changes in security UI implementation: - accepts imgIImageRequest as being subrequest relevant - stores such request in mTransferringRequests hashtable; image requests do not report TRANSFERING state and it is not a simple (even if possible) to allow them to report it and security UI works only with requests that transferred any data This works also for 302 status image redirections. Back-forward issue has already been fixed in another bug.

Comment on attachment 319541 [details] [diff] [review] Fix 1 This patch doesn't work for case #3: request found in cache. there is no channel associated and just OnStopRequest is directly called to notify the image load completeness.

This caches the security info of the channel within cached imgRequest.

this seems reasonable, although if an image is shared between documents, i assume the lock icon will be wrong?

I get a build error with patch v2: /home/kaie/moz/head/mozilla/modules/libpr0n/src/imgRequestProxy.cpp: In member function 'virtual nsresult imgRequestProxy::GetSecurityInfo(nsISupports**)': /home/kaie/moz/head/mozilla/modules/libpr0n/src/imgRequestProxy.cpp:390: error: 'class nsDerivedSafe<imgRequest>' has no member named 'mSecurityInfo'

Added missing header file

Thanks, this patch compiles for me, and it passes my little old test. I guess it would be good to set up a test for Stuart's comment 23.

Stuart, this test page opens (after 1 second) two new windows, which both load the same 3.7 MB image. https://kuix.de/misc/insecure-image/multi.php

Comment on attachment 319802 [details] [diff] [review] Fix 2.1 >- isSubDocumentRelevant = PR_FALSE; >+ nsCOMPtr<imgIRequest> imgRequest(do_QueryInterface(aRequest)); >+ if (!imgRequest) { >+ PR_LOG(gSecureDocLog, PR_LOG_DEBUG, >+ ("SecureUI:%p: OnStateChange: not relevant for sub content\n", this)); >+ isSubDocumentRelevant = PR_FALSE; >+ } else { >+ // For image request emulate data transfer Please say in the comment that we are not getting a transfer notification for images, and possibly refer to an open bug number. Can you please change this code, so we only do this once? I'm worried we do this (unnecessarily) multiple times, once for each kind of notification we get. You told me, the stop notification is the only reliable notification we get? If so, can you please set a boolean here (isImage) and move the PL_DHASH_ADD further down, to the place where we already process the stop request for sub? >+ nsAutoMonitor lock(mMonitor); >+ PL_DHashTableOperate(&mTransferringRequests, aRequest, PL_DHASH_ADD); >+ } > } > } > } > } >

Changed according to Kai's comment.

Comment on attachment 319858 [details] [diff] [review] Fix 3 Look ok to me, thanks! r=kengert for the changes in mozilla/security/manager Before you ask for superreview, you might want to get these portions fixed with up-to-date information: >+++ netwerk/base/public/nsISecurityInfoProvider.idl 2008-05-07 19:25:44.248009400 +0200 >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. >+ * Portions created by the Initial Developer are Copyright (C) 1998 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ *

Just modified the comment in IDL.

Comment on attachment 321736 [details] [diff] [review] Final version >+++ modules/libpr0n/src/imgRequestProxy.cpp >+NS_IMPL_ISUPPORTS4(imgRequestProxy, imgIRequest, nsIRequest, nsISecurityInfoProvider, > nsISupportsPriority) Was there a reason not to put the new interface at the end? >+NS_IMETHODIMP imgRequestProxy::GetSecurityInfo(nsISupports** _retval) >+ *_retval = nsnull; >+ if (mOwner) >+ NS_IF_ADDREF(*_retval = mOwner->mSecurityInfo); Why do the first assignment if mOwner? Also, I would prefer a getter for the security info on the imgRequest over just grabbing its members. Please check with stuart to see which he prefers. >+++ modules/libpr0n/src/imgRequestProxy.h >+class imgRequestProxy : >+ public imgIRequest, >+ public nsISupportsPriority, >+ public nsISecurityInfoProvider I'd leave those hanging off to the right of the ':' unless there's a good reason to reformat this way... >+++ netwerk/base/public/nsISecurityInfoProvider.idl >+ * The Initial Developer of the Original Code is mozilla.org I would think the initial developer is you. >+++ security/manager/boot/src/nsSecureBrowserUIImpl.cpp >+// static >+nsresult >+nsSecureBrowserUIImpl::ExtractSecurityInfo(nsIRequest* aRequest, nsISupports** _retval) This should have the signature: already_AddRefed<nsISupports> ExtractSecurityInfo(nsIRequest* aRequest) since the nsresult is in fact completely pointless and ignored by all callers. >+ // Remeber this is an image request. Because image loads doesn't "remember". Please add a note in bug 432685 explaining what code here needs to be removed once that bug is fixed? >@@ -944,27 +974,32 @@ nsSecureBrowserUIImpl::OnStateChange(nsI >+ if (isImageRequest) >+ requestHasTransferedData = PR_TRUE; >+ else > { I'd prefer curlies around the if body in this case, since the else has them. >@@ -1131,21 +1166,21 @@ nsSecureBrowserUIImpl::OnStateChange(nsI >- >+ Please don't make random whitespace changes? >+++ security/manager/boot/src/nsSecureBrowserUIImpl.h >+ static nsresult ExtractSecurityInfo(nsIRequest* aRequest, nsISupports** _retval); Is there a need for this to be a static class method instead of just a static in the file?

I fixed all bz's comments except the last one - I believe the method should be in class because there are other helper method already.

Comment on attachment 322109 [details] [diff] [review] Final version, 2 s/_retval/retval/ and looks good.

I'm very happy this work is now done, thanks a lot to Honza for working on the patch, and to Boris and Stuart for their reviews. I have pushed the patch to mozilla-central as changeset cf3eae029913. Marking fixed.

I've backed this out since it appears to have caused a Tp regression on OSX http://graphs.mozilla.org/#show=911655&sel=1218767385,1219005881

Dave, what's next? Did you verify/confirm that this patch was responsible for the tp increase? (Furthermore, the increase you quoted was very small. There seems to have been a much higher increase in the recent past, that got accepted?)

(In reply to comment #38) > Dave, what's next? Did you verify/confirm that this patch was responsible for > the tp increase? Yes, we are pretty confident that it was the cause of the Tp regression, and while small it still needs to verified that the fix this patch brings is worth the regression (by module owner/driver types). > (Furthermore, the increase you quoted was very small. There seems to have been > a much higher increase in the recent past, that got accepted?) That hasn't been accepted, it is still under investigation, bug 450401

Do we have any indication which pages actually got slower? Or was it across the board?

I set up https://wiki.mozilla.org/StandaloneTalos on a Mac Mini with OSX 10.4, thanks a lot to Alice for getting me started. I downloaded the nightly Firefox builds from Aug 17 (including the patch) and from Aug 19 (patch backed out). After running standalone talos with both, I did some manual preparation in order to compare the numbers. rm all.csv; for i in tsvg.csv tsspider.csv tp.csv tjss.csv tgfx.csv tdhtml.csv ; do cat $i >> all.csv; done cat all.csv |awk -F, '{print $2 " " $4}' > all2.csv I will attach both versions of all2.csv I used tkdiff to visually compare the numbers from both runs. My manual inspection shows the following pages as the ones with the biggest differences: replaceimages 20% layers5 15% zoom 10% gearflowers 10% imageslide 9% layers1 5% composite-* 0-7%

The numbers are based on the "mean values".

Interesting. Some of those pages load a whole slew of images... Want to try running replaceimages and seeing whether you hit your new code?

No solution yet, this comment only has some description and thoughts around the old and the new code. IIUC, the Tp tests do not involve any SSL. Yesterday I made the statement: "The only new non-SSL action I can see in the patch: Each time we run imgRequest::OnStartRequest we call channel->GetSecurity, which is limited to some addref." Now I realize this was wrong. PSM's state tracking tracks the state of all elements of a page, even if the top page is plain http. That means, all new code of the patch gets executed for non-SSL pages, too. Each time in OnStateChange, each time in OnLocationChange, we attempt to extract the security info from the channel. Without patch, this is limited to nsIRequest->QI->nsIChannel->GetSecurityInfo In addition with the patch, if that operation fails, we attempt nsIRequest->QI->nsIChannel->GetSecurityInfo With the patch, we use QI to detect whether a request is an image request. Both old/new code track the requests that have transferred data in a hashtable. There is a change in our processing of STOP progress events. Without the patch, we always do the hashtable lookup, and if found, remove. However, with the patch, if we know it's an image request, we don't try to remove from the hashtable. But the new code simply assumes we don't see TRANSFERRING notifications. It does not enforce consistency. Should we get such notifications, we'll add the request to the hashtable, but never remove it. This inconsistency should get fixed in a new revision of the patch. I'll test whether we already run into it.

Testing replaceimages.html Without patch, we call OnSecurityChange 6 times. With patch, we call it 3759 times. That should explain the performance regression. We should avoid calling OnSecurityChange when the state doesn't change.

Yes, please! Every OnSecurityChange call hits the UI, running JS and so forth, right?

I have a potential fix in bug 451420. I probably should do a local test with both patches and repeat the local talos test.

Addressing comment 46: "Should we get such notifications, we'll add the request to the hashtable, but never remove it." Also rearranged QI to search for type of the request - imgRequst is tested sooner because it is much often present while common browsing then ftp or other types of requests.

Comment on attachment 334745 [details] [diff] [review] Final version, 3 Your idea to reorder is probably helpful. Your patch does not yet address BZ's review request from comment 34. I previously did before checking in. We should attach a patch that contains that change. r=kaie with that change

Includes that change.

(In reply to comment #49) > I have a potential fix in bug 451420. > I probably should do a local test with both patches and repeat the local talos > test. According to my tests the patch fixes the performance issue.

As the code from this patch overlaps with the proposed code for bug 450912, I'm attaching this merged version. I'll ask for review for the new (overlapping) subset in that bug.

Re-landed, combined with bug 450912 (the functional regression) and bug 451420 (avoid performance regression). With all 3 patches combined, my local performance testing still gave me a slight performance regression on some pages, namely on those that were affected most badly. I haven't yet tried to calculate the overall performance change locally. Let's have a look at the chart (see comment 37) later today or tomorrow.

pushed to hg with revision 2dc9da1db912, marking fixed, second attempt

Fixed before 1.9.1 branched

Verified on a recent Shiretoko build. Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b3pre) Gecko/20090220 Shiretoko/3.1b3pre