It looks like the isSubDocumentRelevant boolean is only checked in one spot in nsSecureBrowserUIImpl::OnStateChange, but is always set. Setting it requires a QI cascade, which is expensive. Is there a reason not to move this code to the one case when the boolean's value is needed? That said, why do we even have this boolean? Why shouldn't a page become insecure if it has a subframe loaded via some random insecure protocol? At the moment it looks like we only do this for http/file/wyciwyg/ftp...

Could someone maybe answer my questions from comment 0 paragraph 2? That looks like a security bug to me, basically...

The code was introduced by me, when I tried to make the (completely broken) original lock icon tracking better. And I think the best thing to do were to work on bug 62178, and follow my suggestions there, to incorporate the lock icon tracking directly into document loading, and not have it rely on some after-the-fact notifications. But I should address your question. There is only one explanation why I could have added the whitelist of protocols: For SSL pages, there were progress events delivered to nsSecureBrowserUIImpl::OnStateChange, where the delivered request was - either not QIable to nsIChannel - or the associated nsIChannel did not carry the state from the SSL session, causing UpdateSubrequestMembers to update the lock icon to insecure I do not remember the type of the disturbing request objects. Maybe a whitelist of disturbing objects, that should be filtered out for security tracking, is more reasonable than the current whitelist of protocols. If you think it is reasonable to improve that logic (because working on the suggested less indirect security tracking is too far away), I suggest to do testing and logging, in order to come up with a list of disturbing objects.

> - either not QIable to nsIChannel These should just be ignored, imo. > - or the associated nsIChannel did not carry the state from the SSL session, Sounds like reflow and parser requests. We've already removed the former and are working on removing the latter; should I revisit this bug at that point? In any case, I just filed this because it was showing up in profiles; if fixing bug 62178 will fix this as well, we should just do that.

The proposed patch for bug 62178 removes the code to QI for isSubDocumentRelevant decision. Only requests having name "about:layout-dummy-request" are continue to be filtered. (Not sure we still get those, that part of the patch was coded 2 years ago).

> Only requests having name "about:layout-dummy-request" are continue to be > filtered. The only such request left is the parser request and that's slated for the chopping block. What about "about:document-onload-blocker"? Should security code be ignoring that? It's used in much the way that about:layout-dummy-request was....

> What about "about:document-onload-blocker"? I have added ignoring these state events to the patch in bug 62178. > It's used in much the way that about:layout-dummy-request was.... Was? I still get them on the trunk, with seamonkey.

It looks like the parser request still claims to be about:layout-dummy-request. We have a bug to remove that; I should just do it.

We no longer have any about:layout-dummy-request requests.

reassign bug owner. mass-update-kaie-20120918

This seems like something we should (and now can) do.