It is possible to see a "broken/mixed security" lock icon and warning, when surfing https pages, that are perfectly secure. However, this does only occur, when one clicks a link, or presses "stop" or "escape" while the page is still loading. The reason for the wrong display comes from the fact, that for summarizing the security info, we are currently analyzing the transfer status of the page's parts. When the user interrupts loading too early, it is possible that loading of some parts has begun, for whom no security information is yet available. On stop, all those connections are stopped, too. The logic tries to extract security information, but is unable to do so. Therefore it counts the objects which were tried to load as being insecure. And having at least one insecure object is enough for the summarized page status going to "mixed". While this is a bug in behaviour, it is not a bug in code. Rather the code is currently not trying to detect this kind of failure. Expected behaviour: Interrupted loading of objects on a page should not lead to mixed/broken security icon. Suggested implementation: For each loaded subpart of a displayed page, the code should take into consideration two more properties: a) the loading "stop reason" b) the fact, whether content bytes have been transfered or not. We have to prove, but actually I believe, that a) is not necessary, looking at b) should be enough. I suspect, security information will be unavailable for those connections, where the SSL handshaking did not yet finish. Those connections did not yet transfer any content data, and therefore could not lead to insecure data being displayed. Therefore those connections should get ignored for determining page security status. Therefore it should be enough to implement a "per-object flag" whether bytes were received already or not yet at all. It has to be researched, whether web progress notification (the mechanism used by PSM to track security state) already delivers the information or not.
Assignee: ssaux → kaie
Priority: -- → P2
Whiteboard: [adt2 RTM]
Target Milestone: --- → 2.3
I think I have a solution for this problem, my tests so far show that I have a working fix. Here are two test cases: - https://www.kuix.de/misc/test16/something.html - https://www.kuix.de/misc/test16/nothing.html Both testcases are frame-documents. The left frame will not immediately, but there will be a 10 second delay before the page is completely shown. The first testcase "something" uses more text content in the left frame, there will be some content shown in the page before the 10 second timeout is reached. This testcase has already been working correctly. When you hit stop, we correctly find security information for the frame, and the lock icon will indicate a secure page. The second testcase "nothing" will display nothing in the frame if you hit cancel early. Right now, and this is what this bug complains about, when you hit cancel before the 10 second timeout, the page will go to a mixed/insecure state. The reasons for the failure is indeed, as I explained in the previous comments, that no data has been transfered over the channel yet, and we are unable to retrieve security information. But because there really has no data been transfered, there will nothing be shown on the screen that could potentially been insecure. So we can ignore the state of this frame, even though we are unable to obtain security information for it. I'll be attaching a patch that corrects the problem. For each part of the loaded document, we remember whether data has been transfered or not, and ignore the requests we don't care for.
Status: NEW → ASSIGNED
Created attachment 85952 [details] [diff] [review] Same Patch, but ignoring whitespace changes for easier reviewing
Javi, can you please review?
let's get this to bake on the trunk, have banks who reported being worried about this bug give it a spin. We should make sure that we review a build that contains both this fix and the fix to bug 148610.
Comment on attachment 85951 [details] [diff] [review] Suggested Fix I was told nsHashtable is not recommended for usage. I'll rework the patch to use a better hashtable class.
Attachment #85951 - Flags: needs-work+
Attachment #85952 - Attachment is obsolete: true
More testing showed that my patch does not work correctly. I assumed that I would see the STATE_TRANSFERRING state for any request part for the loaded document. But I don't see that request for subdocuments like CSS or JS documents loaded over separate URLs. So I guess I have to use something else.
Rick, do you have an idea? How could I query a nsIRequest at the time I receive the STOP progress, whether any content bytes have been transfered or not?
My previous comment might rise the impression, there is a partial solution, even if bug 148981 is not fixed. Thinking about it more, I would like to restate, there is no partial solution. The approach of this suggested patch only work, if one can rely one STATE_TRANSFERRING events to arrive. If we can't rely on that, we must assume the worst case, check for security information, and this bug will not be fixed in any way. To restate what our options are: Either: - Fix for 148981 and a statement from the owner of the document loader team, that we can indeed rely on STATE_TRANSFERING objects to arive, for any request where content bytes have been transfered Or: - Do not rely on that event. Instead, find a way to obtain the information, by querying an object passed in to the progress listener, whether content bytes have been transfered or not.
Created attachment 86207 [details] [diff] [review] Updated Patch / Only recommended with 148981 fixed
Attachment #85951 - Attachment is obsolete: true
Created attachment 86208 [details] [diff] [review] Updated Patch / ignoring whitespace for easier review / Only recommended with 148981 fixed
The previous patch would make the redness go away, but it would be a regression without having 148981 fixed. Because we would display a page as secure, even if stylesheets (and potential other subdocuments) have been transfered without security.
*** Bug 125199 has been marked as a duplicate of this bug. ***
I'm dumping some thoughts about my patch (only of interest for those trying to follow the implementation). While a document gets loaded, I need to keep a list of all the requests that were seen. The list gets cleared each time a new top level document loading starts. For each request, I remember a bool, whether data has been transfered or not. Actually, I'm simplifying, by only remembering those URLs, where data has actually been transfered. The existence in the list defines that data has been transfered for that request. Loading a document can consist of a lot of requests. Because I don't want to be responsible for a performance regression, or cause other side effects by delaying the freeing of request objects, I made a simplification in the previous version of my patch. I'm not storing references to the objects, and I'm not storing URLs, I'm just storing the numerical pointer values to the request objects. But when I did that, I blindly assumed that those pointer values would not get reused while loading a document. But I think I should not assume that. If a request finishes and gets deleted early, the address might get re-used. I thought I would have to use a more complicated solution, but actually there is a simple one: I don't need to remember an object longer than it's lifetime. I know that I will receive a stop event before an object gets deleted. The solution is: Remove the entry from the hash table, as soon as the state has been looked up. That way, we only remember pointer variables during the lifetime of request objects, and we are fine by making that simplification. The next issue with my previous patch is that I limited the detection to sub-documents only. However, as seen in bug 125199 - see also the testcase at https://www.kuix.de/misc/test21/ - it makes sense to include the top level document case. I will attach a new patch that implements the above changes.
Created attachment 88228 [details] [diff] [review] Patch v3 / needs 148981
Attachment #86207 - Attachment is obsolete: true
Created attachment 88229 [details] [diff] [review] Patch v3 / ignoring whitespace / needs 148981
Attachment #86208 - Attachment is obsolete: true
javi: Can you please review? jkeiser: You asked me to use the PLD hash table. Can you please have a look whether I'm using it correctly? Thanks!
Comment on attachment 88228 [details] [diff] [review] Patch v3 / needs 148981 r=javi
Attachment #88228 - Flags: review+
Looks good to me, except the ADD and REMOVE don't require a lookup first. ADD will do a lookup automatically and REMOVE will just do nothing if the entry isn't there.
Created attachment 88393 [details] [diff] [review] Patch v4 / needs 148981 Slightly updated patch, only removing the unnecessary lookup-before-add. I think I can't remove the lookup-before-remove, because I need the information whether the key was contained in the table or not.
Attachment #88228 - Attachment is obsolete: true
Created attachment 88394 [details] [diff] [review] Patch v4 / ignoring whitespace / needs 148981 Same patch, ignoring whitespace.
Attachment #88229 - Attachment is obsolete: true
Comment on attachment 88393 [details] [diff] [review] Patch v4 / needs 148981 carrying forward r=javi.
Attachment #88393 - Flags: review+
I just retested, the patch from this bug, combined with the patch from bug 148981, also corrects the issues with german TOPSITE https://freemail.web.de, as reported in duplicate bug 125199.
Comment on attachment 88394 [details] [diff] [review] Patch v4 / ignoring whitespace / needs 148981 email@example.com
Attachment #88394 - Flags: superreview+
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Keywords: adt1.0.1, mozilla1.0.1
Resolution: --- → FIXED
junruh - can you pls verify this fix on the trunk, then resolve this bug as verified? thanks!
Verified on 6/21 trunk. The lock icon behavior has never been more accurate.
Status: RESOLVED → VERIFIED
Comment on attachment 88393 [details] [diff] [review] Patch v4 / needs 148981 To avoid confusion during the approval phase, carrying over sr= from the white-space-ignoring patch to this one.
Attachment #88393 - Flags: superreview+
Comment on attachment 88394 [details] [diff] [review] Patch v4 / ignoring whitespace / needs 148981 Marking this patch as obsolete, since it was only a helper for easier reviewing.
Attachment #88394 - Attachment is obsolete: true
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending drivers approval. pls check this in asap, then add the "fixed1.0.1" keyword.
Keywords: adt1.0.1 → adt1.0.1+, approval
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Checked in to 1_0 branch.
Keywords: adt1.0.1+, mozilla1.0.1+ → fixed1.0.1
Verified on 6/25 branch.
Keywords: fixed1.0.1 → verified1.0.1
*** Bug 100787 has been marked as a duplicate of this bug. ***
*** Bug 153962 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.