397492 - Consider filtering redundant calls to onSecurityChange

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Johnathan and I have noticed that onSecurityChange seems to be called rather frequently during "normal" page loads, even though the security state hasn't changed in a noticeable way. This makes us update browser chrome elements more than we need to, which has an impact on page load times.

As an experiment, I tried caching the current URI, the "state" parameter to onSecurityChange, and the "SSLStatus" object, and returned early from the main browser window's onSecurityChange if they hadn't changed since the last call. When running a patch with this changed applied through the Tp pageset (40 pages visited 5 times each, all non-SSL), I observed a total of ~600 calls to onSecurityChange, ~400 of which returned early due to the change I made.

From reading the comment at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp&rev=1.65#518 it seems to me that the backend code that fires onSecurityChange must deal with a large number of edge cases (frames, subelements, etc) and therefore errs on the side of caution, firing onSecurityChange in cases where it can't be sure that something hasn't changed. Given that we know exactly what information the browser code uses to update it's security UI, we should be able to avoid redundant UI updates by ignoring onSecurityChange notifications when that information hasn't changed.

Another question is whether this filtering should be done at a lower level than the browser front end code. We could perhaps do it in the browser status filter (nsBrowserStatusFilter) and share the benefit with other users of that class (i.e. SeaMonkey). It may also be possible to reduce the frequency of these notifications at the source (PSM code), but I fear that change code at that low of a level introduces more regression risk given the potential impacts on other consumers.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
> As an experiment, I tried caching the current URI, the "state" parameter to
> onSecurityChange, and the "SSLStatus" object, and returned early from the main
> browser window's onSecurityChange if they hadn't changed since the last call.
> When running a patch with this changed applied through the Tp pageset (40 pages
> visited 5 times each, all non-SSL), I observed a total of ~600 calls to
> onSecurityChange, ~400 of which returned early due to the change I made.

This test was flawed, I now realize, because most of the resources were served from the same host. I took another random sample of 100 non-SSL pages (using http://random.yahoo.com/bin/ryl this time), and the results were similar: only 99 calls out of 527 were needed to make the same UI changes we currently make.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch I'm using to test

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

I think this is the approach to take for now. Filtering at a lower level would potentially gain us more, but it would also affect more code, and it's impossible to know what a suitable subset of information to cache is given that anyone can add a WPL to the tabbrowser.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 282443 [details] [diff] [review]
patch

r=mano

Comment 5

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/base/content/browser.js 	1.853

Comment 6

[Boris Zbarsky [:bzbarsky]]

Neil, we might want something similar in seamonkey too.