Closed Bug 1333682 Opened 7 years ago Closed 7 years ago

[Form Autofill] Prevent duplicate autocomplete search registration

Categories

(Toolkit :: Form Manager, defect)

defect
Not set
normal

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
(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.
(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 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+
The commit message is updated now, thank you Matt :)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/7cde5062bfea
Status: ASSIGNED → RESOLVED
Closed: 7 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
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
https://hg.mozilla.org/mozilla-central/rev/eab0586ca1ec
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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+
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
https://hg.mozilla.org/mozilla-central/rev/ba0d5d08105d
Status: REOPENED → RESOLVED
Closed: 7 years ago7 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.

Attachment

General

Created:
Updated:
Size: