Closed Bug 1304001 Opened 4 years ago Closed 2 years ago

LoginHelper.createLogger is watching and overwriting the same prefs multiple times

Categories

(Toolkit :: Password Manager, defect, P4)

Product:
Toolkit
Component:
Password Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: johannh, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [passwords:tech-debt])

Attachments

(6 files)

Bug 1304001 - Update LoginHelper prefs even if no logger was created. r=sfoster
47 bytes, text/x-phabricator-request
Details | Review
Bug 1304001 - Update comments related to gEnabled/rememberSignons. r=sfoster
47 bytes, text/x-phabricator-request
Details | Review
Bug 1304001 - Move LoginUtils._getPasswordOrigin to LoginHelper. r=sfoster
47 bytes, text/x-phabricator-request
Details | Review
Bug 1304001 - Move LoginUtils._getActionOrigin to LoginHelper. r=sfoster
47 bytes, text/x-phabricator-request
Details | Review
Bug 1304001 - Stop passing the top window to LoginManagerContent. r=sfoster
47 bytes, text/x-phabricator-request
Details | Review
Bug 1304001 - Remove unused 'blur' event listener. r=sfoster
47 bytes, text/x-phabricator-request
Details | Review 
In http://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/LoginHelper.jsm#55 we listen to "signon." and overwrite the internal preference fields according to the changes. However, LoginHelper.createLogger is called several times and so we end up with many identical listeners to the same preferences.

We probably won't be able to get rid of the listener in that function entirely, because we still want to set the maxLogLevel from signon.debug, but we should at least factor out listening to the other prefs.
P4 since these pref changes are infrequent
Priority: -- → P4
Whiteboard: [passwords:tech-debt]
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Blocks: 1529025

This means there is only one signon.* listener for the whole process, not per-logger.

Fixes some stales comments and identation.

Depends on D20391

There were too many top-level objects in that large JSM and LoginHelper didn't exist when it was added.

Depends on D20392

It wasn't clear in callee code that the window was the top-window and it wasn't necessary in many cases. Relying on the top-window would also cause problems with Fission if the windows are in separate processes.

Depends on D20394

The blur event doesn't bubble so this wouldn't actually listen to fields getting blurred. See bug 1138774.

Depends on D20395

Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/6346331d934d
Update LoginHelper prefs even if no logger was created. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/e41015881591
Update comments related to gEnabled/rememberSignons. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/93bd4d634b14
Move LoginUtils._getPasswordOrigin to LoginHelper. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/ed7ae6b877df
Move LoginUtils._getActionOrigin to LoginHelper. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/d75340a9a264
Stop passing the top window to LoginManagerContent. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/5fafa838de11
Remove unused 'blur' event listener. r=sfoster

Comment on attachment 9045044 [details]
Bug 1304001 - Remove unused 'blur' event listener. r=sfoster

Revision D20396 was moved to bug 1529700. Setting attachment 9045044 [details] to obsolete.

Attachment #9045044 - Attachment is obsolete: true

I moved the blur event listener removal to bug 1529700 since it caused the dom/ test failure.

Flags: needinfo?(MattN+bmo)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/e23ef4dbbc7f
Update LoginHelper prefs even if no logger was created. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/a5644de3c2f5
Update comments related to gEnabled/rememberSignons. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/7d8f97779a35
Move LoginUtils._getPasswordOrigin to LoginHelper. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/c9628639b5e1
Move LoginUtils._getActionOrigin to LoginHelper. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/275c3744e059
Stop passing the top window to LoginManagerContent. r=sfoster
Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c320a8b15dfc
Backed out 5 changesets for dom/html/test/test_bug430351.html failures CLOSED TREE

Backed out 5 changesets (Bug 1304001) for dom/html/test/test_bug430351.html failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.10%2Cdebug%2Cmochitests%2Cwith%2Ce10s%2Ctest-macosx64%2Fdebug-mochitest-e10s-2%2Cm-e10s%282%29&fromchange=9316cb12864b8c9b377de978ad22b861567cd4bd&tochange=903a04ef7adcece77a10381b169c40b0831aa529

Backout link: https://hg.mozilla.org/integration/autoland/rev/c320a8b15dfc10065c8606baba0d8a2fdb0bfbc1

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=229785497&repo=autoland&lineNumber=11645

00:41:36 INFO - TEST-START | dom/html/test/test_bug428135.xhtml
00:41:36 INFO - GECKO(915) | ++DOMWINDOW == 69 (0x10a18e800) [pid = 917] [serial = 400] [outer = 0x11af03400]
00:41:36 INFO - GECKO(915) | MEMORY STAT | vsize 4128MB | residentFast 164MB | heapAllocated 24MB
00:41:36 INFO - TEST-OK | dom/html/test/test_bug428135.xhtml | took 180ms
00:41:36 INFO - GECKO(915) | ++DOMWINDOW == 70 (0x111438000) [pid = 917] [serial = 401] [outer = 0x11af03400]
00:41:36 INFO - TEST-START | dom/html/test/test_bug430351.html
00:41:36 INFO - GECKO(915) | ++DOMWINDOW == 71 (0x11143b800) [pid = 917] [serial = 402] [outer = 0x11af03400]
00:41:36 INFO - GECKO(915) | ++DOCSHELL 0x111458000 == 3 [pid = 917] [id = {46927e10-7fc5-1c44-97e5-0e8a0ca4a2b3}]
...
00:41:37 INFO - TEST-PASS | dom/html/test/test_bug430351.html | <iframe xmlns="http://www.w3.org/1999/xhtml" src="about:blank" tabindex="1"></iframe> in <div xmlns="http://www.w3.org/1999/xhtml" id="parent"><iframe src="about:blank" tabindex="1"></iframe></div> should be focusable
00:41:37 INFO - TEST-PASS | dom/html/test/test_bug430351.html | <iframe xmlns="http://www.w3.org/1999/xhtml" src="about:blank" contenteditable="true"></iframe> in <div xmlns="http://www.w3.org/1999/xhtml" id="parent"><iframe src="about:blank" contenteditable="true"></iframe></div> should be focusable
00:41:37 INFO - Buffered messages finished
00:41:37 INFO - TEST-UNEXPECTED-FAIL | dom/html/test/test_bug430351.html | <iframe xmlns="http://www.w3.org/1999/xhtml"></iframe> in <div xmlns="http://www.w3.org/1999/xhtml" id="parent"><iframe></iframe></div> should be focusable - got [object HTMLBodyElement], expected [object HTMLIFrameElement]
00:41:37 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:320:16
00:41:37 INFO - testElements@dom/html/test/test_bug430351.html:480:13
00:41:37 INFO - test@dom/html/test/test_bug430351.html:497:5
00:41:37 INFO - rval@http://mochi.test:8888/MochiKit/DOM.js:604:34
00:41:37 INFO - EventHandlerNonNulladdToCallStack@http://mochi.test:8888/MochiKit/DOM.js:632:13
00:41:37 INFO - addLoadEvent@http://mochi.test:8888/MochiKit/DOM.js:640:14
00:41:37 INFO - @SimpleTest/SimpleTest.js:1453:5
00:41:37 INFO - GECKO(915) | [Child 917, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005: file /builds/worker/workspace/build/src/docshell/shistory/nsSHistory.cpp, line 1204
00:41:37 INFO - GECKO(915) | ++DOCSHELL 0x11e41e800 == 13 [pid = 917] [id = {1641d1a2-e157-f244-b2e3-10d235d6cd20}]
00:41:37 INFO - GECKO(915) | ++DOMWINDOW == 90 (0x11c298800) [pid = 917] [serial = 421] [outer = 0x0]
00:41:37 INFO - Not taking screenshot here: see the one that was previously logged
00:41:37 INFO - TEST-UNEXPECTED-FAIL | dom/html/test/test_bug430351.html | <iframe xmlns="http://www.w3.org/1999/xhtml" tabindex="-1"></iframe> in <div xmlns="http://www.w3.org/1999/xhtml" id="parent"><iframe tabindex="-1"></iframe></div> should be focusable - got [object HTMLBodyElement], expected [object HTMLIFrameElement]
00:41:37 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:320:16
00:41:37 INFO - testElements@dom/html/test/test_bug430351.html:480:13
00:41:37 INFO - test@dom/html/test/test_bug430351.html:497:5
00:41:37 INFO - rval@http://mochi.test:8888/MochiKit/DOM.js:604:34
00:41:37 INFO - EventHandlerNonNulladdToCallStack@http://mochi.test:8888/MochiKit/DOM.js:632:13
00:41:37 INFO - addLoadEvent@http://mochi.test:8888/MochiKit/DOM.js:640:14
00:41:37 INFO - @SimpleTest/SimpleTest.js:1453:5
00:41:37 INFO - GECKO(915) | [Child 917, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005: file /builds/worker/workspace/build/src/docshell/shistory/nsSHistory.cpp, line 1204
00:41:37 INFO - GECKO(915) | ++DOCSHELL 0x11e420000 == 14 [pid = 917] [id = {f16b9f06-4727-e046-aec1-93ae54230898}]
00:41:37 INFO - GECKO(915) | ++DOMWINDOW == 91 (0x11c299400) [pid = 917] [serial = 422] [outer = 0x0]
00:41:37 INFO - Not taking screenshot here: see the one that was previously logged

Flags: needinfo?(MattN+bmo)
Attachment #9045044 - Attachment is obsolete: false

I guess I bisected wrong… I reverted some of the patch to continue using the top window for onDOMInputPasswordAdded and that seems to fix it locally for me.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e079032c995401c705c883ea97ac53ff8b483b47

Flags: needinfo?(MattN+bmo)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/61b426c73ccf
Update LoginHelper prefs even if no logger was created. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/06877b0fd1a9
Update comments related to gEnabled/rememberSignons. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/e25db31b4a15
Move LoginUtils._getPasswordOrigin to LoginHelper. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/32398fe05ec2
Move LoginUtils._getActionOrigin to LoginHelper. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/d6646400f45d
Stop passing the top window to LoginManagerContent. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/275692bc35bc
Remove unused 'blur' event listener. r=sfoster
Depends on: 1541625
Regressions: 1555317
You need to log in before you can comment on or make changes to this bug.