Transfer mode of images should be relevant for shown lock icon state (mixed content)

RESOLVED FIXED in mozilla1.9.1a2

Status

defect
P1
normal
RESOLVED FIXED
18 years ago
3 years ago

People

(Reporter: kaie, Assigned: mayhemer)

Tracking

(Depends on 1 bug, Blocks 2 bugs, {verified1.9.1})

Other Branch
mozilla1.9.1a2
Dependency tree / graph
Bug Flags:
blocking1.8b5 -
blocking1.9.1 +
blocking1.9 -
blocking1.8.1 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low] [sg:want p1][kerh-coa], )

Attachments

(4 attachments, 10 obsolete attachments)

10.92 KB, text/plain
Details
10.96 KB, text/plain
Details
19.43 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
22.33 KB, patch
Details | Diff | Splinter Review
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.
Depends on: 135011
Blocks: lockicon
Keywords: nsbeta1
Priority: -- → P1
Summary: Transfer mode of images should be relevant for shown lock icon state → Transfer mode of images should be relevant for shown lock icon state
*** 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. ***
Flags: blocking-aviary1.1?
Product: PSM → Core
Assignee: kaie → dveditz
*** Bug 219502 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Flags: blocking-aviary1.1- → blocking-aviary1.1?
Target Milestone: --- → mozilla1.8beta4
*** Bug 301275 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1? → blocking1.8b4+
is this being worked on?  if not, who should be working on it?  (or should this
be deferred?)

/cb
Whiteboard: [at risk?] [eta?]
should be looked at.  not going to make 1.8b4.  deferring.

/cb
Flags: blocking1.8b4+ → blocking1.8b4-
Whiteboard: [at risk?] [eta?]
Blocks: 305284
Blocks: 305282
Whiteboard: [sg:fix]
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.
Whiteboard: [sg:fix] → [sg:fix] [kerh-coa]
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.
Posted patch experimental code (obsolete) — Splinter Review
Whiteboard: [sg:fix] [kerh-coa] → [sg:low] [kerh-coa]
Flags: blocking1.9a1?
Flags: blocking1.8.1?
blocking: integrity of the lock icon is important.
Flags: blocking1.8.1? → blocking1.8.1+
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.
Target Milestone: mozilla1.8beta4 → mozilla1.8.1beta1
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.
Flags: blocking1.8.1+ → blocking1.8.1-
Assignee: dveditz → kengert
Blocks: 321022
Flags: blocking1.9a1? → blocking1.9-
*** 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?
QA Contact: junruh → ui
Summary: Transfer mode of images should be relevant for shown lock icon state → Transfer mode of images should be relevant for shown lock icon state (mixed content)
Blocks: newlock
Posted patch Fix 1 (obsolete) — Splinter Review
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.
Attachment #205293 - Attachment is obsolete: true
Attachment #319541 - Flags: review?(kengert)
Assignee: kengert → honzab
Status: NEW → ASSIGNED
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.
Attachment #319541 - Flags: review?(kengert)
Posted patch Fix 2 (obsolete) — Splinter Review
This caches the security info of the channel within cached imgRequest.
Attachment #319541 - Attachment is obsolete: true
Attachment #319593 - Flags: review?(kengert)
Attachment #319593 - Flags: review?(pavlov)
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'
Posted patch Fix 2.1 (obsolete) — Splinter Review
Added missing header file
Attachment #319593 - Attachment is obsolete: true
Attachment #319802 - Flags: review?(pavlov)
Attachment #319802 - Flags: review?(kengert)
Attachment #319593 - Flags: review?(pavlov)
Attachment #319593 - Flags: review?(kengert)
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);
>+          }
>         }
>       }
>     }
>   }
>
Attachment #319802 - Flags: review?(kengert) → review-
Depends on: 432685
Posted patch Fix 3 (obsolete) — Splinter Review
Changed according to Kai's comment.
Attachment #319802 - Attachment is obsolete: true
Attachment #319858 - Flags: review?(pavlov)
Attachment #319858 - Flags: review?(kengert)
Attachment #319802 - Flags: review?(pavlov)
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):
>+ *
Attachment #319858 - Flags: review?(kengert) → review+
Posted patch Final version (obsolete) — Splinter Review
Just modified the comment in IDL.
Attachment #321736 - Flags: approval1.9?
Attachment #321736 - Flags: approval1.9? → superreview?(bzbarsky)
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?
Posted patch Final version, 2 (obsolete) — Splinter Review
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.
Attachment #319858 - Attachment is obsolete: true
Attachment #321736 - Attachment is obsolete: true
Attachment #322109 - Flags: superreview?(bzbarsky)
Attachment #322109 - Flags: review?(pavlov)
Attachment #319858 - Flags: review?(pavlov)
Attachment #321736 - Flags: superreview?(bzbarsky)
Comment on attachment 322109 [details] [diff] [review]
Final version, 2

s/_retval/retval/ and looks good.
Attachment #322109 - Flags: superreview?(bzbarsky) → superreview+
Duplicate of this bug: 434945
Flags: blocking1.9.1?
Whiteboard: [sg:low] [kerh-coa] → [sg:low] [sg:want p1][kerh-coa]
Attachment #322109 - Flags: review?(pavlov) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8.1beta1 → mozilla1.9.1a2
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Depends on: 451420
I have a potential fix in bug 451420.
I probably should do a local test with both patches and repeat the local talos test.
Posted patch Final version, 3 (obsolete) — Splinter Review
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.
Attachment #322109 - Attachment is obsolete: true
Attachment #334745 - Flags: review?(kaie)
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
Attachment #334745 - Flags: review?(kaie) → review+
Includes that change.
Attachment #334745 - Attachment is obsolete: true
Attachment #334767 - Flags: review+
(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.
Blocks: 452401
Attachment #335640 - Attachment is obsolete: true
Attachment #335692 - Attachment is obsolete: true
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
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Fixed before 1.9.1 branched
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1
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
Duplicate of this bug: 490506
Depends on: 519776
Depends on: 749946
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.