Closed Bug 832834 Opened 8 years ago Closed 2 years ago

nsSecureBrowserUIImpl makes babies and kittens cry


(Core :: Security: PSM, defect, P1)




Tracking Status
firefox64 --- fixed


(Reporter: Dolske, Assigned: keeler)


(Depends on 1 open bug)


(Whiteboard: [psm-assigned])


(1 file)

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.
Depends on: 832835
Depends on: 832837
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
Depends on: 1127158
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+
Pushed by
reimplement nsSecureBrowserUIImpl r=franziskus,Felipe
Backout by
Backed out changeset 4ad33c6fbfca for merge conflict. CLOSED TREE
Pushed by
reimplement nsSecureBrowserUIImpl r=franziskus,Felipe
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1491543
Duplicate of this bug: 1211646
Depends on: 1535851
Duplicate of this bug: 703510
No longer depends on: 1535851
Regressions: 1535851
No longer depends on: 1490982
Regressions: 1490982
No longer depends on: 1491543, 1493427, 1495321
Regressions: 1491543, 1493427, 1495321
No longer depends on: 304848
Duplicate of this bug: 304848
You need to log in before you can comment on or make changes to this bug.