1382548 - Sending message that cannot be cloned at FormAutofillContent.jsm:161:6

Status: VERIFIED FIXED

Description

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Initially we were explicitly avoiding this issue by not sending the element references but then I was told that we no longer needed to avoid this warning and that special-casing was removed. It seems like we're sending XPCOM objects again…

> Sending message that cannot be cloned. Are you trying to send an XPCOM object? FormAutofillContent.jsm:161:6

https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/browser/extensions/formautofill/FormAutofillContent.jsm#161

Comment 1

[Luke Chang [:lchang] (inactive)]

Created attachment 8902527 [details]
Bug 1382548 - [Form Autofill] Avoid sending element references in messages.

Review commit: https://reviewboard.mozilla.org/r/174124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174124/

Comment 2

[Ray Lin[:ralin]]

Comment on attachment 8902527 [details]
Bug 1382548 - [Form Autofill] Avoid sending element references in messages.

https://reviewboard.mozilla.org/r/174124/#review179366

LGTM, thanks.

I'm thinking if we can add an unit test in _getRecords using like sinon.matcher to verify whether the element reference is passed?

Comment 3

[Ray Lin[:ralin]]

Ignore what I just said :P

It seems unnecessary as that's unlikely be regressed by other patches in the future, plus, even though it doesn't cause any fatal error.

Comment 4

[Luke Chang [:lchang] (inactive)]

Thanks.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0357cf75ec0c
[Form Autofill] Avoid sending element references in messages. r=ralin

Comment 6

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/0357cf75ec0c

Comment 7

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Comment on attachment 8902527 [details]
Bug 1382548 - [Form Autofill] Avoid sending element references in messages.

Approval Request Comment
[Feature/Bug causing the regression]: Not sure of the specific bug but it was a month or so ago
[User impact if declined]: Not really user-facing but it will spam Fx/addon (not web) developer  consoles when autofill is used
[Is this code covered by automated tests?]: Yes but it's harmless in this case. Automated tests would catch cases where it isn't harmless
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just cloning an object and deleting the element reference from the clone before sending across the process boundary
[String changes made/needed]: None

Comment 8

[Liz Henry (:lizzard) (use needinfo)]

Assuming from MattN's request that this affects 56 (since we are launching the feature in 56)

Comment 9

[Liz Henry (:lizzard) (use needinfo)]

Comment on attachment 8902527 [details]
Bug 1382548 - [Form Autofill] Avoid sending element references in messages.

Avoid sending pointless log messages, let's uplift. 
We are hitting some errors with the form autofill uplifts related to the differences in preferences - not sure if this will suffer from the same issue or not but if so it may need rebasing for beta.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/577b2db694a7

Comment 11

[Cornel Ionce [:cornel_ionce], Desktop Release QA]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #7)
> [Needs manual test from QE? If yes, steps to reproduce]: No
PEr Matthew's assessment, this does not require manual testing. Marking as qe-verify-.