1394140 - WebExtension experiment schema should be loaded before a dependent extension's manifest is parsed

Status: NEW

(WebExtensions :: General, enhancement, P5)

Description

[Rob Wu [:robwu]]

Due to a flaw in the load order of WebExtension experiments (WEE), the the schema as declared by the WEE is not available when the first extension that depends on the 
WEE is loaded. Consequently, a manifest warning appears when the dependent extension is loaded, and the manifest is not validated against the schema.

The flow below shows how an extension is loaded (by Extension.jsm).
The bug is caused by the fact that step 3 executes before step 5, i.e. manifest normalization/validation is performed before the schema of the WEE is loaded.

1. loadManifest (Extension's) is called, which calls loadManifest (superclass ExtensionData's)
- https://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/toolkit/components/extensions/Extension.jsm#1008-1009
- https://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/toolkit/components/extensions/Extension.jsm#603-605

2. manifest.json is read + built-in extension APIs are initialized if needed.
- https://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/toolkit/components/extensions/Extension.jsm#486-490

3. Manifest is validated against the known schemas.
- https://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/toolkit/components/extensions/Extension.jsm#526-530

4. Dependency on experimental WebExtension is detected if present in the manifest.
- https://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/toolkit/components/extensions/Extension.jsm#561-591

5. At this point the manifest has been parsed and loadManifest of the superclass (ExtensionData) returns.
   The subclass (Extension) now loads experimental dependencies (API implementation AND schema).
- https://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/toolkit/components/extensions/Extension.jsm#1015-1023
- https://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/toolkit/components/extensions/ExtensionCommon.jsm#115-147



A way to fix this is to load the schemas of the WEE before step 3 (these are static JSON files, so the performance impact ought to be minimal).
Regardless of how this bug is fixed, to prevent regressions we need a unit test where a WEE adds schema validation, and two dependent extensions: one with a correct manifest file, and one where the experimental manifest key is in the incorrect format.

Comment 1

[Rob Wu [:robwu]]

Attachment: someExample.xpi

STR (Nightly only because WEE can only be used in Nightly):
1. If using Firefox 57, set extensions.legacy.enabled=true (because of bug 1394121).
2. Load the attached someExample.xpi at about:debugging.
3. Extract the attached sampleWebExtension.zip and load it at about:debugging.

Expected:
- The console contains: "Result of async someExample.getMyProperty(): some value in manifest.json"
- There are no warnings at about:debugging.

Actual:
- The console contains the expected message.
- Unexpectedly, about:debugging shows the following warning:
  "Reading manifest: Error processing your_custom_property_here: An unexpected property was found in the WebExtension manifest."

Comment 2

[Rob Wu [:robwu]]

Attachment: sampleWebExtension.zip

This is the demo extension used in comment 1 for STR.

Comment 3

[Rob Wu [:robwu]]

Attachment: sampleWebExtension-invalid-manifest.zip

This is the same extension, except the manifest value is deliberately of the wrong type.

When the extension is loaded for the first time, there is no validation error,
but
"Reading manifest: Error processing your_custom_property_here: An unexpected property was found in the WebExtension manifest."

When the extension is loaded again or reloaded, then the extension fails to (re)load and the following error appears in the global console:
"ERROR Loading extension 'null': Reading manifest: Error processing your_custom_property_here.some_required_property: Expected string instead of 123"




In the last case, the demo extension does not produce any console messages, which indicates that the background page did not (re)load.

In the previous test cases, the demo extension prints the following message:
Result of someExample.getMyPropertySync(): undefined  background.js:8:5
Result of async someExample.getMyProperty(): 123  background.js:2:5