Closed Bug 451420 Opened 11 years ago Closed 11 years ago
Avoid calling On
Security Change unnecessarily
The code in nsSecureBrowserUIImpl calls OnSecurityChange very often, and it does so unnecessarily. This was exposed in bug 135007. See bug 135007 comment 37 up to at least bug 135007 comment 47.
We've noticed this before, and tried to improve the situation in front end code in bug 397492 and bug 398360. I'm happy you've made progress investigating this in PSM code - perhaps we can remove some of the complexity added to front-end code by those patches once this patch lands.
landed on trunk, d5a093d32472
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
backed out, reopening. TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug420160.js | Identity handler is getting the full location - Got undefined, expected test1.example.org TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug420160.js | getEffectiveHost should return example.org for test1.example.org - Got undefined, expected example.org TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug420160.js | Identity handler is getting port information - Got undefined, expected sub1.test1.example.org:8000 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug420160.js | getEffectiveHost should return example.org for sub1.test1.example.org:8000 - Got undefined, expected example.org TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug420160.js | getEffectiveHost should return 127.0.0.1 for 127.0.0.1:8888 - Got undefined, expected 127.0.0.1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm able to see the test failure locally. cd obj-fire-debug Xvfb -ac :1 & DISPLAY=:1 nice python runtests.py --browser-chrome --log-file=log --file-level=INFO --console-level=DEBUG --autorun --close-when-done
This updated patch passes the --browser-chrome test My first patch was too aggressive. It limited notifications to the real state changes. But the consumers expect us to post the event on other events, at least for on location changes. I've changed the patch to send out notifications, in addition, whenever we change to a new location, update the tooltip or the ssl status object. According to the replaceimages test, we still send out the notification only a few times. I uploaded the test to my server, in order to be able to test over https. As that didn't give me the usual animation (changing images), I added a bigger timeout to ensure we indeed change the images. http://kuix.de/mozilla/test_dhtml/replaceimages.html https://kuix.de/mozilla/test_dhtml/replaceimages.html https://kuix.de/mozilla/test_dhtml/replaceimages_slow.html
This patch was supposed to improve our performance. But with the standalone-talos test I see a 1.5 % runtime increase on the replaceimages test. When combining all patches from bug 135007, bug 451420, bug 450912, I get a total performance regression of 3.2 % on replaceimages. The above numbers are based on a comparison test with no patch applied. Of course, this is better than our original increase of 20.3 % on the replaceimages test.
You see a 1.5% regression with just this patch and nothing else? That seems pretty odd...
Correct, I don't understand it either (yet). I might re-run the test.
I repeated the with-patch and without-patch test. I'm glad to report that the results are now exactly opposite. With patch applied the times are lower, so I had obviously mixed up the files from the earlier test... i,page, median,mean,min,max,runs 12,replaceimages.html, with-old-135007/tdhtml.csv:1195.5,1194.75,1194,1229,1229|1196|1195|1194|1194 without-451420-2/tdhtml.csv:1015.5,1004.25,991,1040,1004|991|995|1040|1027 with-451420-2/tdhtml.csv:993,992.5,989,1025,1025|989|995|991|995 with-135007-450912-451420/tdhtml.csv:1025.5,1022.25,1018,1042,1042|1027|1018|1020|1024 0,gearflowers.svg, with-old-135007/tsvg.csv:2225,2133.75,1988,2693,2693|2103|2347|2097|1988 without-451420-2/tsvg.csv:1962.5,1911,1852,2371,2371|1889|1867|1852|2036 with-451420-2/tsvg.csv:1946,1911.75,1874,2687,2687|1881|2010|1874|1882 with-135007-450912-451420/tsvg.csv:1943.5,1897.5,1849,2514,2514|1868|1854|2019|1849 3,imageslide.html, with-old-135007/tdhtml.csv:397.5,380.25,359,420,379|367|416|420|359 without-451420-2/tdhtml.csv:412.5,389,364,609,609|413|412|364|367 with-451420-2/tdhtml.csv:366.5,364.75,361,418,365|367|418|366|361 with-135007-450912-451420/tdhtml.csv:415.5,403.75,369,643,643|415|416|415|369 7,layers5.html, with-old-135007/tdhtml.csv:597.5,596.5,595,613,596|613|598|597|595 without-451420-2/tdhtml.csv:529,522.75,516,854,516|854|524|517|534 with-451420-2/tdhtml.csv:517.5,516.25,513,530,517|530|517|513|518 with-135007-450912-451420/tdhtml.csv:537.5,534.25,530,622,622|530|532|536|539
Comment on attachment 335690 [details] [diff] [review] Patch v2 r+ with one comment. It appears that you could get away with one Bool in UpdateSecurityState since the only testing of the 3 bools is in a single if or'd together. I presume the reason for 3 separate ones is either: 1) flexibility in the future (you may need to have different logic for the various cases). 2) Ease in reading the code... (the knowledge of withNewLocation || withUpdateStatus || withUpdateTooltip means "TellTheWorld" is limited to UpdateSecurityState and not each of its callers. Other note: There's and existing API confusion in that it's not obvious that showWarning is a Bool reference and can be changes. Maybe comment where it's called would be in order (This is a preexisting condition).
Attachment #335690 - Flags: review?(rrelyea) → review+
pushed to hg with revision 98cc97026fef, marking fixed. I had talked to Bob, he is fine to keep the 3 separate bools, as they are good for self-documenting, and we might want to change behavior based on individual bools in the future. Bob's optional proposal to document code was about existing code, I will do an incremental patch.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
I pushed this patch with revision 9599ebdf16b0
You need to log in before you can comment on or make changes to this bug.