Closed Bug 451420 Opened 11 years ago Closed 11 years ago

Avoid calling OnSecurityChange unnecessarily

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: kaie, Assigned: kaie)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #334720 - Flags: review?(rrelyea)
Attachment #334720 - Flags: review?(rrelyea) → review+
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
Attached patch Patch v2Splinter Review
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
Attachment #334720 - Attachment is obsolete: true
Attachment #335690 - Flags: review?(rrelyea)
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 ago11 years ago
Resolution: --- → FIXED
I pushed this patch with revision 9599ebdf16b0
Depends on: 749946
You need to log in before you can comment on or make changes to this bug.