Closed Bug 1495908 Opened Last year Closed 3 months ago

Label unexpected WebExtension manifest.json properties as a Warning

Categories

(WebExtensions :: Compatibility, defect, P3)

defect

Tracking

(firefox71 verified)

VERIFIED FIXED
mozilla71
Tracking Status
firefox71 --- verified

People

(Reporter: ianbicking, Assigned: myeongjun.ko, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Unexpected values in manifest.json, including properties that are supported in Chrome but not in Firefox, give messages like "Error processing version_name: An unexpected property was found in the WebExtension manifest."

This gives the impression that the error is concerning, when it is probably fine, especially because unexpected properties are ignored, while "error" implies the add-on might not load. Simply renaming this as "Warning" may keep people from worrying. Bug 1495888 is an example of this overconcern.
Status: NEW → UNCONFIRMED
Component: Untriaged → Compatibility
Ever confirmed: false
QA Contact: ddurst
QA Contact: ddurst
Keywords: good-first-bug
Priority: -- → P3
If this is your first contribution, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for instructions on how to get started. 

Mentor: :rpl
Mentor: lgreco
Hi, could I be assigned to this bug?
Welcome, Ammar! The bug's yours. :) Please needinfo Luca (:rpl) with any specific questions that come up or when you're ready to have your patch reviewed.
Assignee: nobody → ammar.akhtar.01
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hi Luca, I need some help finding the file to edit.

I tried running grep and got these two files, but I don't think they are what I need:

```
ammar@ammar-VirtualBox:~/src/mozilla-unified$ grep -rl "An unexpected property was found in the WebExtension manifest." --exclude-dir=obj-x86_64-pc-linux-gnu
toolkit/components/extensions/schemas/manifest.json
toolkit/components/extensions/test/xpcshell/test_ext_schema.js

```
Flags: needinfo?(lgreco)
(In reply to Ammar Akhtar from comment #4)
> Hi Luca, I need some help finding the file to edit.

Hi Ammar,
the reason why the grep commmand is not finding any useful file is that those error messages are being created from the schema validation, in particular the validation warnings related to the "An unexpected property was found in the WebExtension manifest." message are being triggered from here:

- https://searchfox.org/mozilla-central/rev/adec563403271e78d1a057259b3e17fe557dfd91/toolkit/components/extensions/Schemas.jsm#1126-1138

And they use the message that is set in the manifest.json schema file (which the grep command has been already able to find for you):

- https://searchfox.org/mozilla-central/search?q=An+unexpected+property+was+found+in+the+WebExtension+manifest&path=toolkit%2Fcomponents%2Fextensions%2Fschemas%2Fmanifest.json

At line 1140 of Schemas.jsm, logDeprecated calls context.logError to collect the deprecated validation message:

- https://searchfox.org/mozilla-central/rev/adec563403271e78d1a057259b3e17fe557dfd91/toolkit/components/extensions/Schemas.jsm#1140

The logError function called there is defined inside ExtensionData parseManifest method:

- https://searchfox.org/mozilla-central/rev/adec563403271e78d1a057259b3e17fe557dfd91/toolkit/components/extensions/Extension.jsm#574-576

And it looks that it is only meant to log warnings, whereas any "blocking manifest errors" ("blocking" as in "the extension will fail to install successfully") are collected here:

- https://searchfox.org/mozilla-central/rev/adec563403271e78d1a057259b3e17fe557dfd91/toolkit/components/extensions/Extension.jsm#600-603 

And so it seems that the ExtensionData class methods manifestWarning and manifestError may likely be the ones that we are interesting in (as they currently looks like where we know if the error message is a manifest warning or a manifest error):

- https://searchfox.org/mozilla-central/rev/adec563403271e78d1a057259b3e17fe557dfd91/toolkit/components/extensions/Extension.jsm#333,341

Let me know if these additional details are being helpful to you.
Flags: needinfo?(lgreco)

Hey Ammar! How's it going with this bug?

Flags: needinfo?(ammar.akhtar.01)

Hey Ammar, since we haven't heard from you in awhile we are going to open this bug up to other contributors. If you'd still like to work on it, let us know!

Assignee: ammar.akhtar.01 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ammar.akhtar.01)

Hello, could I take this bug?

Hey Josoe, go for it! If you need any help, please needinfo :rpl.

Assignee: nobody → jssantosqueiroz

Hi, I found the line of code were the error string is builded.

Looking at the aplication behavior I've seem that the browser defines semanticaly the exception as a warning. So I think now I need to change the string returned at the error function.

Please tell me if I miss something.

Flags: needinfo?(lgreco)

(In reply to Josoe Santos Queiroz from comment #10)

Hi, I found the line of code were the error string is builded.

That code is used for much more than what we need for this bug, it doesn't look like a good place to apply the changes needed for this issue.

Did you looked to comment 5?
In that comment I provided some details about how the manifest errors and warning are being generated and collected (with also links to the related pieces of code).

Flags: needinfo?(lgreco)

In comment 5 you said I should look at the manifestWarning method. So I faced some paths: the first is to create an other method like the error which I marked in comment 9 but handling exceptions that do not block the extension for running. Or need to just change the string in the manifestWarnig function to something like: Reading manifest: ${message} the extension may do not run as expected.

Flags: needinfo?(lgreco)

Hi Josoe,
looking into how we reach manifestWarning is helpful because we can better asses what kind of changes to apply, the method you mentioned in comment 10 is definitely one of those involved, but as I mentioned in comment 11 it is also used for much more that the deprecation warning and so we need to also find the right place to change so that we can make that method aware if its message parameter should be formatted as an error or as a warning.

While exploring the possible implementation strategies, I usually prefer to have at least one test case, because it makes it easier to investigate the behaviors I'm interesting in, and then double-check how the changes I'm applying are affecting those behaviors
(and it also helps with the final patch, as it should usually contain the necessary test coverage for the changes applied).

step 1 - define a new test case

The following patch defines a new small test case that triggers a manifest warning and then checks the warning messages collected:

diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_manifest.js b/toolkit/components/extensions/test/xpcshell/test_ext_manifest.js
--- a/toolkit/components/extensions/test/xpcshell/test_ext_manifest.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_manifest.js
@@ -41,3 +41,29 @@ add_task(async function test_manifest() 
     });
   }
 });
+
+add_task(async function test_manifest_warnings_on_unexpected_props() {
+  let extension = await ExtensionTestUtils.loadExtension({
+    manifest: {
+      background: {
+        scripts: ["bg.js"],
+        wrong_prop: true,
+      },
+    },
+    files: {
+      "bg.js": "",
+    },
+  });
+
+  await extension.startup();
+
+  // Retrieve the warning message collected by the Extension class
+  // packagingWarning method.
+  const {warnings} = extension.extension;
+  equal(warnings.length, 1, "Got the expected number of manifest warnings");
+
+  const expectedMessage = "Reading manifest: Warning processing background.wrong_prop";
+  ok(warnings[0].startsWith(expectedMessage, "Got the expected warning message format"));
+
+  await extension.unload();
+});

you can apply this patch locally by storing it in a file (e.g. one named "Bug1495908_test_case.patch"), and then run the patch command from the command line while the current directory is your mozilla-central clone (as in patch -p1 < ./Bug1495908_test_case.patch).

Once the patch has been applied (and the new test case added to the test_ext_manifest.js test file), you can run this test file locally using the following command:

./mach xpcshell-test toolkit/components/extensions/test/xpcshell/test_ext_manifest.js

And clearly we expect it to fail, until we have applied the necessary changes to make it run successfully.

step 2 - trace the manifestWarning calls using console.trace

Using the new test case, we can more easily take a look to the code involved and evaluate how we may want to change the implementation.

One easy way to trace the calls to manifestWarning is temporarily add a console.trace call into the manifestWarning method and then run the test file again. You should get more or less the following trace:

console.trace: WebExtensions:
  _modules/Extension.jsm 355 manifestWarning
  _modules/Extension.jsm 609 logError
  _modules/Schemas.jsm 1109 logDeprecation
  _modules/Schemas.jsm 1121 checkDeprecated
  _modules/Schemas.jsm 1255 normalize
  _modules/Schemas.jsm 1366 normalize
  _modules/Schemas.jsm 1742 normalize/r<
  _modules/Schemas.jsm 544 withPath
  _modules/Schemas.jsm 1742 normalize
  _modules/Schemas.jsm 1294
  _modules/Schemas.jsm 506 withChoices
  _modules/Schemas.jsm 1292 normalize
  _modules/Schemas.jsm 1666 checkProperty/r<
  _modules/Schemas.jsm 544 withPath
  _modules/Schemas.jsm 1666 checkProperty
  _modules/Schemas.jsm 1727 normalize
  _modules/Schemas.jsm 3041 normalize
  _modules/Schemas.jsm 3203 normalize
  _dules/Extension.jsm 618 _getNormalizedManifest
  _dules/Extension.jsm 650 parseManifest

From the above trace we can see that logDeprecation may be a good hook point, as it is a place where we know for sure that the error that we are going to log is related to a deprecation (and so it is the kind of message that we want to make it clear that it is a warning message).

step 3 - look into Schemas.jsm logDeprecation

While keeping in mind the trace log we got in the previous step, let's look more deeply into what logDeprecation and logError are doing internally, so that we can start to plan which kind of changes we may need to apply to them to achieve our goal.

logDeprecation currently looks like:

  logDeprecation(context, value = null) {
    let message = "This property is deprecated";
    ...
    context.logError(context.makeError(message));
  }

and so logDeprecation is going to:

  • format the error using context.makeError
  • and then log it using context.logError (which is defined in Extension.jsm and it calls manifestWarning).

Internally context.makeError is calling the context.error(...) function, which is exactly the function that you mentioned in comment 10, the function where the final error message is build.

As I mentioned in Comment 11, these functions (context.makeError and context.error) are used for more then just the manifest warnings, and so we can't just change them to always format the error as "Warning processing...", because the other errors emitted by these functions (besides the ones related to the deprecated properties) should still be formatted as "Error processing...".

Nevertheless, you were right that this may be part of the functions that we may have to apply some changes on.

e.g. The following should be a reasonable approach:

  1. change logDeprecation to pass an additional parameter to context.makeError, to make it aware when a message is actually related to a warning instead of an error, e.g. something like:
  logDeprecation(context, value = null) {
    let message = "This property is deprecated";
    ...
    context.logError(context.makeError(message, {warning: true}));
  }
  1. from context.makeError we could pass the optional parameter (the one that contains the warning property) to context.error (e.g. as its third optional parameter).

  2. in context.error we can use the new optional parameter to decide if the error message should be prefixed with "Error processing" or "Warning processing" (and when this new optional parameter is omitted, context.error should behave like it does now)

Flags: needinfo?(lgreco)

Ok, I did it. As recommended in comment 13, I passed a warning to the context.makeError function and added a condition in the context.error method.

Now the context.makeError method looks like:

  makeError(message, {warning}) {
    let error = forceString(this.error(message, undefined, warning).error);
  ...
  return error;
  }

And the context.error looks like this:

  error(errorMessage, choicesMessage = undefined, warning = false) {
    if (choicesMessage !== null) {
      let {choicePath} = this;
      if (choicePath) {
        choicesMessage = `.${choicePath} must ${choicesMessage}`;
      }

      this.currentChoices.add(choicesMessage);
    }
    if (this.currentTarget) {
      let {currentTarget} = this;
      if (warning) {
        return {error: () => `Warning processing ${currentTarget}: ${forceString(errorMessage)}`};
      }
      return {error: () => `Error processing ${currentTarget}: ${forceString(errorMessage)}`};
    }
    return {error: errorMessage};
  }

So, what is next?

Flags: needinfo?(lgreco)

(In reply to Josoe Santos Queiroz from comment #14)

So, what is next?

Let's put all the pieces together in a properly submitted patch.

You may find the following section of the "Contribution On Ramp" wiki page useful to get your development environment ready for that:

Flags: needinfo?(lgreco)

Hey Josoe, since we haven't heard from you in awhile, we're going to open this bug up for other contributors. You are welcome to come back and continue working on it!

For other contributors: if you would like to work on this, please feel free say hi and ask questions. We will formally assign you to the bug once you have submitted a patch.

Assignee: jssantosqueiroz → nobody

Hi I am Vishal,
I was looking to fix this bug ,
I went through all the comments. But couldn't understand much but see through all the links provided in comments 5 i think the problem might be around the if statements
logDeprecation(context, value = null) {
let message = "This property is deprecated";
if (typeof(this.deprecated) == "string") {
message = this.deprecated;
if (message.includes("${value}")) {
try {
value = JSON.stringify(value);
} catch (e) {
value = String(value);
}
message = message.replace(/${value}/g, () => value);
}
}

 context.logError(context.makeError(message));

}

the message unexpected property is probably because of the value , what i mean is that suppose value is not passed then it will be assigned with null value then it wouldn't go in the if(message.include("${value}")) thus making message value property null and probably
Could you please help me proceed

Flags: needinfo?(lgreco)

Hey Vishal, like Luca said in comment 15, these are the docs for creating the patch. If you could go ahead and create the patch in Phabricator, that would be great (and it will be easier to review and give feedback). Don't worry about breaking anything -- the patch still needs to be reviewed before it can land. :)

Flags: needinfo?(lgreco)

Anyone isn't submit patch about this bug. I wanna solve this bug and submit patch :)

Go for it, Myeongjun!

Assignee: nobody → myeongjun.ko
Keywords: checkin-needed

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a2553a970189
Label unexpected WebExtension manifest.json properties as a Warning r=rpl

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/e7a0ffe6c833
Port bug 1495908 - Label unexpected WebExtension manifest.json properties as a Warning; rs=bustage-fix

Verified the fix using the latest Nightly (71.0a1/20190903215623) under Windows 10 Pro 64-bit and macOS High Sierra 10.13.6.

Loading an extension containing proprieties specific to Chrome will now generate a warning message instead of the previously labeled error message, confirming the fix.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.