[Form Autofill] Prevent duplicate autocomplete search registration

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ralin, Assigned: MattN)

Tracking

(Blocks 1 bug)

Trunk
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [form autofill:M1])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
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
(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

2 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

2 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

2 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

2 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

2 years ago
The commit message is updated now, thank you Matt :)
Keywords: checkin-needed

Comment 11

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7cde5062bfea
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Resolution: FIXED → ---
Summary: [Form Autofill] Prevent duplicate add-on registration → [Form Autofill] Prevent duplicate autocomplete search registration
Target Milestone: mozilla54 → ---
Version: 53 Branch → Trunk

Comment 15

2 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
Blocks: 1339007
Attachment #8830170 - Attachment is obsolete: true

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eab0586ca1ec
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request)
Comment hidden (obsolete)

Comment 20

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba0d5d08105d
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
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.