Closed Bug 1527828 Opened 9 months ago Closed Last month

Remove insecure password field detection code for the address bar

Categories

(Toolkit :: Password Manager, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Fission Milestone M4
Tracking Status
firefox71 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 3 open bugs)

Details

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

Attachments

(2 files)

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

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: → 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.

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
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
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.