nsSecureBrowserUIImpl makes babies and kittens cry

RESOLVED FIXED in Firefox 64

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
13 days ago

People

(Reporter: Dolske, Assigned: keeler)

Tracking

(Depends on 2 bugs)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
One of insights from bug 822371 is that nsSecureBrowserUIImpl is a giant frickin' pain in the keister. In the modern world, it's trying to do too much, crosses layers it doesn't need to, doesn't integrate well with the rest of the stack, and generally makes young children and kittens cry. Please, think of the kittens. And the kids too. Whatever.

Alternatively: security (SSL) is an integral piece of the modern web, the crypto export restrictions of yore are generally long-gone, and we should strive for a cleaner implementation that integrates SSL state/UI more closely.

This is a tracking bug for the various pieces.

So far, I see 3 major pieces to this work:

1) Simplify docshell.securityUI creation and ownership. It's currently a bizarre 3-way dance between the frontend, PSM, and docshell.

2) Spin off the "insecure form submission" stuff to a separate toolkit module. There's little reason for it to live deep down in PSM code.

3) Simplify security state tracking between docshell and PSM code. When tanvi/bsmith/I talked about this, we agreed we want to keep most of this centralized (since it's a bit complex). But there's a _lot_ of code here for managing a few bits of a bitmask, and it seems likely there's a lot of room for improvement. See also, the throttling issues mentioned in bug 822371.
(Reporter)

Updated

6 years ago
Depends on: 832835
(Reporter)

Updated

6 years ago
Depends on: 832837
(Reporter)

Updated

6 years ago
Depends on: 832848
I am started to dig into this stuff and it's indeed a very big mess. For example nsSecureBrowserUIImpl listens for progress changes in the docShell, waiting for StateChange, only to check the security state of the channel and then sending a OnSecurityChange itself.

We need to remove all the touchpoints with docShell for e10s, so I will look into simplifying this.
Depends on: 1110835
Depends on: 1110935
Blocks: 1111908
Assignee: nobody → mgoodwin

Updated

4 years ago
Depends on: 1127158

Updated

4 years ago
Depends on: 1222754
Assignee: mgoodwin → nobody
Component: Security: UI → Security: PSM
Priority: -- → P3
Whiteboard: [psm-backlog]
Assignee: nobody → dkeeler
Priority: P3 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
It turns out nsSecureBrowserUIImpl is considerably more complicated than it
needs to be. This patch reimplements it in terms of OnLocationChange only, which
is all it needs to produce the same behavior as before.
Comment on attachment 9001761 [details]
bug 832834 - reimplement nsSecureBrowserUIImpl r?franziskus,jaws

Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision.
Attachment #9001761 - Flags: review+
Comment on attachment 9001761 [details]
bug 832834 - reimplement nsSecureBrowserUIImpl r?franziskus,jaws

:Felipe Gomes (needinfo me!) has approved the revision.
Attachment #9001761 - Flags: review+

Comment 6

8 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ad33c6fbfca
reimplement nsSecureBrowserUIImpl r=franziskus,Felipe

Comment 7

8 months ago
Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a4e8a30cd2f
Backed out changeset 4ad33c6fbfca for merge conflict. CLOSED TREE

Comment 8

8 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f4adf14e623
reimplement nsSecureBrowserUIImpl r=franziskus,Felipe

Comment 9

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2f4adf14e623
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Updated

7 months ago
Depends on: 1491543
Duplicate of this bug: 1211646

Updated

a month ago
Depends on: 1535851
Duplicate of this bug: 703510
No longer depends on: 1535851
Regressions: 1535851
You need to log in before you can comment on or make changes to this bug.