1890021 - Password manager plain mochitests rely on the JSON fallback for structured cloning

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Andrew McCreight [:mccr8]]

The Mochitest plain password manager tests in toolkit/components/passwordmgr/test/mochitest/ rely on a proxy implementation of LoginManager that forwards requests to the parent process using JS IPC, implemented in pwmgr_common.js. This code is being run as content JS because this is in a plain Mochitest, but it is calling into the chrome JS function LoginHelper.loginToVanillaObject(). That function returns a chrome JS object, so it ends up in a cross-compartment wrapper (CCW). The code then attempts to send the CCW over JS actor IPC, via JSActor::SendQuery. That method attempts to structured clone the CCW by calling into nsFrameMessageManager::GetParamsForMessage(), but structured cloning can't deal with a CCW to a plain object, so it falls back to the stringify JSON method. JS_Stringify apparently can deal with a CCW, so it produces a JSON string which gets turned into a new JS value, which can be successfully structured cloned.

The goal of this patch is to avoid relying on that JSON fallback for structured clone. It does this by reimplementing loginToVanillaObject in content JS, so the resulting object is content JS, so we avoid the CCW.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1890021 - Reimplement loginToVanillaObject in content JS.

This code is being run as content JS because this is in a plain Mochitest, but
it is calling into the chrome JS function LoginHelper.loginToVanillaObject().
That function returns a chrome JS object, so it ends up in a cross-compartment
wrapper (CCW). The code then attempts to send the CCW over JS actor IPC, via
JSActor::SendQuery. That method attempts to structured clone the CCW by calling
into nsFrameMessageManager::GetParamsForMessage(), but structured cloning
can't deal with a CCW to a plain object, so it falls back to the stringify
JSON method. JS_Stringify apparently can deal with a CCW, so it produces
a JSON string which gets turned into a new JS value, which can be successfully
structured cloned.

The goal of this patch is to avoid relying on that JSON fallback for structured
clone. It does this by reimplementing loginToVanillaObject in content JS, so the
resulting object is content JS, so we avoid the CCW.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1564b30ff723
Reimplement loginToVanillaObject in content JS. r=credential-management-reviewers,sgalich

Comment 3

[Steve Fink [:sfink] [:s:]]

but structured cloning can't deal with a CCW to a plain object

Can you clarify? $JS -e 'g = newGlobal({newCompartment:true}); g.eval("o = {a: 100}"); o = deserialize(serialize(g.o)); print(uneval(o))' shows

({a:100})

as I would expect. The spec doesn't know anything about CCWs (other than some things failing if crossing a disallowed security boundary), so I would expect CCWs to work. What isn't working with a CCW? Is there a CheckedUnwrap() or AutoRealm missing in the C++ serialization code somewhere?

Comment 4

[Andrew McCreight [:mccr8]]

I think the actual situation is a little more complex, though I'm not sure if it matters. The serialization is running in the chrome global, on an object built in the chrome global like { id, name, message }, where id and name are values from the chrome global. message is a value from the content global, which is something like {args: [blah], loginInfoIndices, methodName}, where loginInfoIndices and methodName are also from the content global. [blah] is an array, constructed in the content global, containing a single element blah.

Without my patch blah is a plain object from the chrome global (I think it is the same chrome global...), and serialization fails. With my patch, blah is a plain object from the content global and serialization succeeds.

Comment 5

[Andrew McCreight [:mccr8]]

I guess I'm not even sure what kind of wrapper is used to allow content to access the chrome object so maybe something is going wrong when it attempts to climb back through.

Comment 6

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/1564b30ff723

Comment 7

[Andrew McCreight [:mccr8]]

For future reference, here are the tests that seems to hit this issue:
toolkit/components/passwordmgr/test/mochitest/test_autofill_https_downgrade.html
toolkit/components/passwordmgr/test/mochitest/test_autofill_https_upgrade.html
toolkit/components/passwordmgr/test/mochitest/test_autofill_different_formActionOrigin.html
toolkit/components/passwordmgr/test/mochitest/test_autofill_different_subdomain.html
toolkit/components/passwordmgr/test/mochitest/test_autofill_tab_between_fields.htm