451420 - Avoid calling OnSecurityChange unnecessarily

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert [:KaiE:]]

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.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 2

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

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.

Comment 3

[Kai Engert [:KaiE:]]

landed on trunk, d5a093d32472

Comment 4

[Kai Engert [:KaiE:]]

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

Comment 5

[Kai Engert [:KaiE:]]

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

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Patch v2

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

Comment 7

[Kai Engert [:KaiE:]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

You see a 1.5% regression with just this patch and nothing else?  That seems pretty odd...

Comment 9

[Kai Engert [:KaiE:]]

Correct, I don't understand it either (yet).
I might re-run the test.

Comment 10

[Kai Engert [:KaiE:]]

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 11

[Robert Relyea]

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).

Comment 12

[Kai Engert [:KaiE:]]

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.

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Add-On Patch, comment for showWarning

I pushed this patch with revision 9599ebdf16b0