Closed Bug 1527828 Opened 6 years ago Closed 5 years ago

Remove insecure password field detection code for the address bar

Categories

(Toolkit :: Password Manager, enhancement, P2)

Product:
Toolkit
Component:
Password Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Project Flags:
Performance Impact medium
Fission Milestone M4
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Keywords: perf, perf:pageload, Whiteboard: [fxperf:p3])

Attachments

(2 files)

Bug 1527828 - Remove insecure password field detection code for the address bar. r=sfoster
(deleted), text/x-phabricator-request
Details
A secure form with an iframe'd insecure form
(deleted), text/html
Details

_detectInsecureFormLikes[1] runs on every pageshow and from the DOMFormHasPassword event and shows up in this profile from smaug: http://bit.ly/2DI53jz

It's also not going to work properly with Fission IIUC since it traverses window.frames.

IMO this logic which is mostly independent of password manager should be rewritten in C++ and work more like the mixed content address bar indicator. That should provide page load speed improvements.

Test pages:

You should see a broken lock icon in the address bar AND the identity panel should say "Logins entered on this page could be compromised" inside.
I don't have a test-page with subframes handy but you can probably embed those test pages in an <iframe> on an http: server.

[1] https://searchfox.org/mozilla-central/rev/01b4b3830ea3cae2e9e431019afa6391b471c6da/toolkit/components/passwordmgr/LoginManagerContent.jsm#483-505

On a related note, the insecure login field console warnings from InsecurePasswordUtils.jsm could also be consolidated with the address bar indicator logic and we could avoid loading that JSM on page load too. There is also telemetry being accumulated on every page load in InsecurePasswordUtils which could have a perf impact.

Blocks: fission-frontend
Whiteboard: [qf][fxperf] → [qf:p2:pageload][fxperf]
Whiteboard: [qf:p2:pageload][fxperf] → [qf:p2:pageload][fxperf:p3]
Fission Milestone: ? → Future

In bug 1555317 comment 27 we are discussing whether we can now remove this code altogether… doing so could help pageload performance and then we wouldn't have to fix it for Fission.

See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1555317

We got the go-ahead to remove this code now (bug 1555317 comment 28) so we don't have to fix it for Fission and we can reduce the work we do on page loads and tab switches.

Summary: Move insecure password field detection for the address bar indicator to C++ and fix it for Fission → Remove insecure password field detection code for the address bar

So is this a dupe of bug 1567827 now? :)

Maybe, this bug has the context for performance and focused on the passwordmgr/ content process portion whereas bug 1567827 is more about the doorhanger parent process portion. They can be done in one bug or separate depending on the dev. who takes it.

Blocks: 1567827
Blocks: 1582112

Deleting this code allows us to delete 3 failing tests in Fission so moving to M4.

Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Fission Milestone: Future → M4

Test page that frames an insecure form at http://http-login.badssl.com/

Attachment #9100961 - Attachment mime type: text/plain → text/html
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/d80e1918c41d Remove insecure password field detection code for the address bar. r=sfoster

https://hg.mozilla.org/mozilla-central/rev/d80e1918c41d

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Flags: qe-verify+
Performance Impact: --- → P2
Keywords: perf:pageload
Whiteboard: [qf:p2:pageload][fxperf:p3] → [fxperf:p3]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: