Closed
Bug 1333682
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Prevent duplicate autocomplete search registration
Categories
(Toolkit :: Form Manager, defect)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: ralin, Assigned: MattN)
References
(Blocks 1 open bug)
Details
(Whiteboard: [form autofill:M1])
Attachments
(1 file, 1 obsolete file)
We should add a check like[0] to prevent duplicate registration in different frames. STR: 1. open https://luke-chang.github.io/autofill-demo/ 2. open https://luke-chang.github.io/autofill-demo/layout.html Result: - Profile Popup doesn't show up on 'https://luke-chang.github.io/autofill-demo/layout.html' - Get error message in Browser ToolBox: NS_ERROR_FACTORY_EXISTS: Component returned failure code: 0xc1f30100 (NS_ERROR_FACTORY_EXISTS) [nsIComponentRegistrar.registerFactory] [0] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/devtools/client/jsonview/converter-sniffer.js#86-90
Comment 1•7 years ago
|
||
(In reply to Ray Lin[:ralin] from comment #0) > STR: > 1. open https://luke-chang.github.io/autofill-demo/ > 2. open https://luke-chang.github.io/autofill-demo/layout.html Note that two links need to be opened in different tabs for reproducing this issue.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to Luke Chang [:lchang] from comment #1) > (In reply to Ray Lin[:ralin] from comment #0) > > STR: > > 1. open https://luke-chang.github.io/autofill-demo/ > > 2. open https://luke-chang.github.io/autofill-demo/layout.html Ah! I didn't make the steps clear, thank you Luke. > > Note that two links need to be opened in different tabs for reproducing this > issue.
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8830170 [details] Bug 1333682 - Don't register form autofill autocomplete if it's already registered. https://reviewboard.mozilla.org/r/107062/#review108180 ::: browser/extensions/formautofill/content/FormAutofillContent.js:177 (Diff revision 1) > - this._classID = proto.classID; > + let registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar); > > let factory = XPCOMUtils._getFactory(targetConstructor); > this._factory = factory; > > - let registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar); > + if (proto.classID && !registrar.isCIDRegistered(proto.classID)) { > + this._classID = proto.classID; Hey Ray, it seems like there's some unnecessary code movement here. Can you revert it please?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8830170 [details] Bug 1333682 - Don't register form autofill autocomplete if it's already registered. https://reviewboard.mozilla.org/r/107062/#review108180 > Hey Ray, it seems like there's some unnecessary code movement here. Can you revert it please? Revert the code movement in diff3, thanks.
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8830170 [details] Bug 1333682 - Don't register form autofill autocomplete if it's already registered. https://reviewboard.mozilla.org/r/107062/#review108820 Nit: the commit message could be improved a bit: Bug 1333682 - Don't register form autofill autocomplete if it's already registered. r=MattN
Attachment #8830170 -
Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request) |
Reporter | ||
Comment 10•7 years ago
|
||
The commit message is updated now, thank you Matt :)
Keywords: checkin-needed
Comment 11•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7cde5062bfea Don't register form autofill autocomplete if it's already registered. r=MattN
Keywords: checkin-needed
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7cde5062bfea
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 13•7 years ago
|
||
After discussing with Sean and Luke we realized that this was just hiding the real problem that were trying to register the autocomplete search implementation in each frame but there can only be one registration per process. This needs a different solution so I will back it out.
Assignee: ralin → MattN+bmo
Status: RESOLVED → REOPENED
status-firefox54:
fixed → ---
Resolution: FIXED → ---
Summary: [Form Autofill] Prevent duplicate add-on registration → [Form Autofill] Prevent duplicate autocomplete search registration
Target Milestone: mozilla54 → ---
Version: 53 Branch → Trunk
Assignee | ||
Comment 14•7 years ago
|
||
Backout try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2c3752dc70fca5c8919801b32707652fa4f11dc
Comment 15•7 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/mozilla-inbound/rev/eab0586ca1ec Backed out changeset 7cde5062bfea because it was just hiding the real problem. r=me
Assignee | ||
Updated•7 years ago
|
Attachment #8830170 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eab0586ca1ec
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request) |
Comment hidden (obsolete) |
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8836634 [details] Bug 1333682 - Split FormAutofillContent.js into a few JSMs shared for the process. https://reviewboard.mozilla.org/r/112026/#review113556 ::: browser/extensions/formautofill/FormAutofillParent.jsm:122 (Diff revision 2) > } else { > - Services.mm.removeMessageListener("FormAutofill:PopulateFieldValues", this); > - Services.mm.removeMessageListener("FormAutofill:GetProfiles", this); > + Services.ppmm.removeMessageListener("FormAutofill:PopulateFieldValues", this); > + Services.ppmm.removeMessageListener("FormAutofill:GetProfiles", this); > } > > Services.mm.broadcastAsyncMessage("FormAutofill:enabledStatus", this._enabled); Use `Services.ppmm.broadcastAsyncMessage` here? ::: browser/extensions/formautofill/FormAutofillContent.jsm:260 (Diff revision 2) > return info; > }, > > _identifyAutofillFields(doc) { > let forms = []; > this._formsDetails = []; This line will clear `_formsDetails` which is shared in one proccess, and this is fine in E10S mode but non-E10S mode. After moving this line in the object definition, it can be fixed, so I would suggest to include this in this patch. ::: browser/extensions/formautofill/test/unit/test_getFormInputDetails.js:2 (Diff revision 2) > "use strict"; > Need /* global FormAutofillContent */ here to resolve eslint error: 'FormAutofillContent' is not defined. ::: browser/extensions/formautofill/test/unit/test_markAsAutofillField.js:2 (Diff revision 2) > "use strict"; > Need /* global FormAutofillContent */ here to resolve eslint error: 'FormAutofillContent' is not defined.
Attachment #8836634 -
Flags: review?(selee) → review+
Comment 21•7 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0d5d08105d Split FormAutofillContent.js into a few JSMs shared for the process. r=seanlee
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ba0d5d08105d
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 23•7 years ago
|
||
As per San-Francisco meeting with :vchen, marking this bug as qe-.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•