Closed
Bug 397492
Opened 17 years ago
Closed 17 years ago
Consider filtering redundant calls to onSecurityChange
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 3 beta1
People
(Reporter: Gavin, Assigned: Gavin)
Details
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
asaf
:
review+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•17 years ago
|
||
(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.
Assignee | ||
Comment 2•17 years ago
|
||
Assignee | ||
Comment 3•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 282443 [details] [diff] [review]
patch
r=mano
Attachment #282443 -
Flags: review?(mano) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #282443 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #282443 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 5•17 years ago
|
||
mozilla/browser/base/content/browser.js 1.853
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M9
Comment 6•17 years ago
|
||
Neil, we might want something similar in seamonkey too.
You need to log in
before you can comment on or make changes to this bug.
Description
•