1960449 - Idiosyncracies of JSON fallback in nsFrameMessageManager::GetParamsForMessage exposed to WebExtensions via APICalls

Status: ASSIGNED

(WebExtensions :: General, defect, P2)

Description

[Andrew McCreight [:mccr8]]

In bug 1885221, I'm rewriting how we serialize JS actor messages in order to enable dynamic typing of actor messages received by the parent process from a child process. One choice this code has to make is what to do when it comes across something that isn't serializable. If it turns that into null, everything is fine. If it turns that into undefined, then a single test fails, specifically the unserializable subcases (tabkey4 and winkey4) of browser_ext_sessions_window_tab_value.js.

I looked into why this was. The WebExtensions "routing layer" sends arguments packed up into an array inside an object, like this: {args: [3, "foo", () => {return null;}]}

Without the changes in my patch, that object gets passed into nsFrameMessageManager::GetParamsForMessage(). The structured clone fails due to the function, so it hits the StringifyJSON fallback, which turns the object into a string, then back into an object and does a structured clone of the new object.

Looking at the documentation for JSON.stringify(), there's special handling for undefined, functions and symbols:

1. If this value is in an array, turn it into null.
2. If this is a property value in an object, omit the property.
3. Otherwise (which in normal stringify is only at the top level), turn it into undefined.

Because the WebExtension argument is in an array, this turns the function into null so we get {args: [3, "foo", null]}. So, that's why we need to make UntypedFromJSVal turn null for this test to pass. This stringify behavior is even described in the documentation.

It doesn't seem great that we need to rely on this behavior, and there can theoretically be some weird "non-local effects" because if any argument can't be structured cloned, then other arguments will start getting passed through the JSON fallback. For instance, I think that if you have two arguments Number.NaN and true, they'll end up unchanged, but if the second argument is a function then suddenly the first argument will end up as null instead of Number.NaN.

Nika also looked at this, and said that maybe this only applies in a few places, like the session setTabValue and setWindowValue methods from the test above, as well as some telemetry scalarSet method but that's a privileged API so maybe we don't have to be worried about regular WebExtensions doing weird things.

Comment 1

[Andrew McCreight [:mccr8]]

Nika had some details here to add if I remember correctly.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

The TL;DR of my thoughts is effectively that parameters from WebExtensions should not be directly embedded into an JSActor IPC message, as that is exposing the specifics of how we serialize JS objects for chrome JS to extensions (including this fallback case for JSON serialization).

We already avoid this in a few places in the WebExtensions code by manually creating and deserializing from a StructuredCloneHolder (e.g. https://searchfox.org/mozilla-central/rev/08b2a1a770688df19a5a43fd89fb63b34bb2d6a6/toolkit/components/extensions/ExtensionParent.sys.mjs#1235-1241 and https://searchfox.org/mozilla-central/rev/08b2a1a770688df19a5a43fd89fb63b34bb2d6a6/toolkit/components/extensions/ExtensionChild.sys.mjs#849 for the async reply to an API call), however we don't do this for arguments passed in (https://searchfox.org/mozilla-central/rev/08b2a1a770688df19a5a43fd89fb63b34bb2d6a6/toolkit/components/extensions/ExtensionChild.sys.mjs#913,947 and https://searchfox.org/mozilla-central/rev/08b2a1a770688df19a5a43fd89fb63b34bb2d6a6/toolkit/components/extensions/ExtensionParent.sys.mjs#1216,1223).

I think we should probably be consistent and also box the arguments into a StructuredCloneHolder (which ensures that structured clone serialization is being used).

FWIW I expect this will cause breakage in the browser_ext_sessions_window_tab_value.js test, as you cannot serialize an object containing a function over structured clone. To handle that, the functions setTabValue and setWindowValue should perhaps be changed to have custom in-content handlers which serialize the argument received from content with JSON.stringify before sending them over IPC to the parent process, rather than after (https://searchfox.org/mozilla-central/rev/08b2a1a770688df19a5a43fd89fb63b34bb2d6a6/browser/components/extensions/parent/ext-sessions.js#248,275).

Comment 3

[Andrew McCreight [:mccr8]]

Rob, can somebody look at fixing this? If the problem I'm describing is clear. Thanks.

Comment 4

[Rob Wu [:robwu]]

So there are two actionable tasks described here:

• comment 2: use StructuredCloneHolder in in callParentAsyncFunction (sender and receiver).
• Put JSON.stringify call for sessions.setTabValue and sessions.setWindowValue in the child.

They both look reasonable to me.

Besides this, we should also check for other uses of "type": "any" in the extension APIs, in particular in values that are inputs from extensions. And then see if we need to do anything special here.

On the sessions API, it would be as trivial as putting a setTabValue and setWindowValue in browser/components/extensions/child/ext-sessions.js. We don't have that file yet, but an external contributor once created a patch that did that at https://phabricator.services.mozilla.com/D168824. If we want to move fast, we could look into merging that patch and building on top of it.

Andrew, how quickly do you want this to be done?

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Rob Wu [:robwu] from comment #4)

Andrew, how quickly do you want this to be done?

Something in the next 2 or 3 weeks would be good, if that sounds reasonable. Thanks.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Adding a needinfo assigned to Tomislav for looking into this.

Comment 7

[Andrew McCreight [:mccr8]]

It has been about a month since I filed this. Any updates here? Thanks.

Comment 8

[Rob Wu [:robwu]]

I audited for uses of "type": "any" as input values to the extension API methods, and the results are as follows:

• (ignoring multiple results where "any" is in an API marked as "unsupported": true)
• (ignoring ImageDataType (example), which is preprocessed by the schema-generated bindings)
• (ignoring multiple results where "any" is a return value or event detail, because in these cases the data flows from the parent to the child, and are already using StructuredCloneHolder).
• (ignoring sendMessage and connect APIs in the runtime, tabs and userScripts namespace because these already use StructuredCloneHolder internally)
• (ignoring privileged APIs in the tree; if desired this can be a separate investigation: pictureinpicture and webcompat, (mobile-only) webcompat-reporter)
• (source) events.Rule type definition (MDN docs. This was copied from Chromium, but is not used anywhere in Firefox.
• (source) extensionsTypes.Date type can take a Date instance with arbitrary properties, where we only care about the date. We don't care about the extra properties, and Date can be structured cloned.
• (source) scripting.executeScript takes an args parameter. This is already correctly serialized in child/ext-scripting.js before passing up to the parent.
• (source) sessions.setTabValue and sessions.setWindowValue take arbitrary values, but should be JSON-serializable values only. See comment 4 for implementation path.
  • ACTION REQUIRED
• (source) storage API has several fields declared any. The implementation is in child/ext-storage.js, which wraps the individual properties in StructuredCloneHolder before sending it to the parent. Also, the other way around does the same.
  • ... this always happens for session, sync, managed (sources), and almost always for local too, except for storage.local.set when the database has not initialized yet (source).
  • ACTION REQUIRED
• (source) The privileged-only telemetry.submitPing takes arbitrary inputs. This API cannot be used by regular extensions.
• (source) The Setting type takes arbitrary inputs. But the specific implementations accept specific formats only, usually primitive values. Extensions should not be passing non-JSON values...
  • ... but we do a very poor job at validating that. E.g. browser.privacy.services.passwordSavingEnabled.set({value:document}) currently throws "An unexpected error occurred", with the Browser Console showing can't set pref signon.rememberSignons to value '[object Object]'; it isn't a String, Number or Boolean. The internals provide validation eventually, but we should be doing this at the extension framework level.
  • ACTION REQUIRED

I also audited uses of isInstanceOf, since these could also be non-plain objects. They are mostly return values or things whose implementation is in the child.

Remaining tasks: Items marked as "ACTION REQUIRED" above.

Comment 9

[Rob Wu [:robwu]]

(In reply to Rob Wu [:robwu] from comment #4)

So there are two actionable tasks described here:

• comment 2: use StructuredCloneHolder in in callParentAsyncFunction (sender and receiver).
• Put JSON.stringify call for sessions.setTabValue and sessions.setWindowValue in the child.

Following my audit in comment 8, it looks like we can try to make sure that all input to individual APIs are properly sanitized at the API binding level. If we can rely on sanitization at the API binding level, we do not need to structurally clone again in callParentAsyncFunction. In bug 1910660 I described that there is already a bunch of overhead in serialization, and it would be nice if we don't serialize when not needed.

Comment 10

[Tomislav Jovanovic :zombie]

Attachment: Bug 1960449 - Properly serialize arguments before calling parent methods r?robwu