Bugzilla

Comment 1

•

6 years ago

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

Ammar Akhtar

Comment 2

•

6 years ago

Hi, could I be assigned to this bug?

BugBot [:suhaib / :marco/ :calixte]

Comment 3

•

6 years ago

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

Updated

•

6 years ago

Status: UNCONFIRMED → ASSIGNED

Ever confirmed: true

Ammar Akhtar

Comment 4

•

5 years ago

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)

Comment 5

•

5 years ago

(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)

Comment 6

•

5 years ago

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

Flags: needinfo?(ammar.akhtar.01)

Comment 7

•

5 years ago

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)

Comment 8

•

5 years ago

Hello, could I take this bug?

Comment 9

•

5 years ago

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

Assignee: nobody → jssantosqueiroz

Comment 10

•

5 years ago

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.

Updated

•

5 years ago

Flags: needinfo?(lgreco)

Comment 11

•

5 years ago

(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)

Comment 12

•

5 years ago

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)

Comment 13

•

5 years ago

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:

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}));
  }

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).
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)

Comment 14

•

5 years ago

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)

Comment 15

•

5 years ago

(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:

https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch

Flags: needinfo?(lgreco)

Comment 16

•

5 years ago

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

Vishal Chaudhary

Comment 17

•

5 years ago

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

Updated

•

5 years ago

Flags: needinfo?(lgreco)