Remove extraneous transferables from "PasswordManager:onFormSubmit" message
Categories
(Toolkit :: Password Manager, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox126 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 3 open bugs)
Details
Attachments
(1 file)
LoginManagerChild.sys.mjs has this piece of code:
this.sendAsyncMessage("PasswordManager:onFormSubmit", {},
{fields, isSubmission, triggeredByFillingGenerated});
At first glance it doesn't look too weird, but the non-empty object is being passed in as a second argument, which means it is being used as a transferables argument for structured cloning. The arguments to the actor sendAsyncMessage() end up in nsFrameMessageManager::GetParamsForMessage() for structured cloning. I'm not sure exactly how that is supposed to work, but because of the bogus transferables argument the Write() call fails, which makes the method go to a backup serialization method that turns the first argument (but not the second) into JSON, parses the JSON into a JS value, and then attempt to serialize that (without the transferables). All we wanted to send was an empty object, so it ends up getting the right thing anyways.
The fix here is to delete that entire second argument.
It looks like this call with the second argument was introduced in bug 1729640. As far as I can tell, it was just a copy paste error that managed to be overlooked because it ends up with the right value in the end.
I came across this because I am looking at changing the behavior of this fallback JSON serialization and a bunch of password manager tests mysteriously broke (browser_autofill_track_filled_logins.js, browser_crossOriginSubmissionUsesCorrectOrigin.js, browser_doorhanger_autofill_then_save_password.js, etc.).
|
Assignee | |
Updated•1 year ago
|
|
|
Assignee | |
Comment 1•1 year ago
|
||
The second argument to sendAsyncMessage is used as a transferables argument during the
structured cloning of the first argument, which causes
nsFrameMessageManager::GetParamsForMessage to fall back to an odd JSON serialization
method which ends up only sending the first argument anyways. This patch drops
the second argument so we only need normal structured cloning to successfully send
this message.
|
||
Comment 2•1 year ago
|
||
Weird, it kind of looks like it intended to send that object as the data, rather than as transferables, though even if you swapped the two the empty object is not an array so it would break the clone. I agree that it definitely seems better to remove it entirely.
|
|
Assignee | |
Comment 3•1 year ago
|
||
Yeah, I assumed it was that, but from what I looked at for the code at the time, I don't think that those fields made much sense. And the way it is now, the receiver just checks that two fields aren't present and doesn't do anything else. My alternative structured clone strategy returns null as a fallback, so the check that a field isn't present causes things to break.
|
||
Comment 5•1 year ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 6•1 year ago
|
||
For future reference, here are some of the tests that failed due to this issue (there were probably more failures in the toolkit/components/passwordmgr/test/browser/ directory)
browser/components/enterprisepolicies/tests/browser/browser_policy_masterpassword_doorhanger.js
toolkit/components/passwordmgr/test/browser/browser_autofill_track_filled_logins.js
toolkit/components/passwordmgr/test/browser/browser_crossOriginSubmissionUsesCorrectOrigin.js
toolkit/components/passwordmgr/test/browser/browser_doorhanger_autofill_then_save_password.js
toolkit/components/passwordmgr/test/browser/browser_doorhanger_crossframe.js
Description
•