LoginHelper.createLogger is watching and overwriting the same prefs multiple times
Categories
(Toolkit :: Password Manager, defect, P4)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: johannh, Assigned: MattN)
References
(Blocks 1 open bug)
Details
(Whiteboard: [passwords:tech-debt])
Attachments
(6 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
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.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 2•2 years ago
|
||
This means there is only one signon.* listener for the whole process, not per-logger.
Assignee | ||
Comment 3•2 years ago
|
||
Fixes some stales comments and identation.
Depends on D20391
Assignee | ||
Comment 4•2 years ago
|
||
There were too many top-level objects in that large JSM and LoginHelper didn't exist when it was added.
Depends on D20392
Assignee | ||
Comment 5•2 years ago
|
||
Depends on D20393
Assignee | ||
Comment 6•2 years ago
|
||
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
Assignee | ||
Comment 7•2 years ago
|
||
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 9•2 years ago
|
||
Backed out 6 changesets (Bug 1304001) for mochitest failures in test_bug430351.html CLOSED TREE
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=229513641&repo=autoland&lineNumber=11697
Backout: https://hg.mozilla.org/integration/autoland/rev/fa83ce0044782353ca970a16da016f4a01a449cd
Comment 10•2 years ago
|
||
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.
Assignee | ||
Comment 11•2 years ago
|
||
I moved the blur
event listener removal to bug 1529700 since it caused the dom/ test failure.
Comment 12•2 years ago
|
||
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
Comment 13•2 years ago
|
||
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
Comment 14•2 years ago
|
||
Backed out 5 changesets (Bug 1304001) for dom/html/test/test_bug430351.html failures
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
Updated•2 years ago
|
Assignee | ||
Comment 15•2 years ago
|
||
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
Assignee | ||
Updated•2 years ago
|
Comment 16•2 years ago
|
||
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
Comment 17•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/61b426c73ccf
https://hg.mozilla.org/mozilla-central/rev/06877b0fd1a9
https://hg.mozilla.org/mozilla-central/rev/e25db31b4a15
https://hg.mozilla.org/mozilla-central/rev/32398fe05ec2
https://hg.mozilla.org/mozilla-central/rev/d6646400f45d
https://hg.mozilla.org/mozilla-central/rev/275692bc35bc
Comment hidden (obsolete) |
Description
•