[Form Autofill] Prevent duplicate autocomplete search registration

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
Form Manager
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: ralin, Assigned: MattN)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 months 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

Comment 1

5 months 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

5 months 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
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

5 months 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.
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

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

Comment 11

5 months 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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7cde5062bfea
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox54: --- → fixed
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
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
Backout try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2c3752dc70fca5c8919801b32707652fa4f11dc

Comment 15

4 months 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

Updated

4 months ago
Blocks: 1339007
Attachment #8830170 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 17

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eab0586ca1ec
Status: REOPENED → RESOLVED
Last Resolved: 5 months ago4 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request)
Blocks: 1300989
Comment hidden (obsolete)

Comment 20

4 months 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

4 months 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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba0d5d08105d
Status: REOPENED → RESOLVED
Last Resolved: 4 months ago4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.