Don't log warning when `background.persistent` is set to `true`

VERIFIED FIXED in Firefox 66

Status

defect
P5
normal
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: e7358d9c, Assigned: edward.i.wu, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

unspecified
mozilla66
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox65 wontfix, firefox66 verified)

Details

(Whiteboard: triaged)

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170518000419

Steps to reproduce:

Create and load a WebExtension with a background page that has the `background.persistent` property set to `true`.

A test extension is attached as an attachment.


Actual results:

The following warning message was printed into the console:
```
addons.webextension.persistent-background-page@bmo-test-extension	WARN	Loading extension 'persistent-background-page@bmo-test-extension': Reading manifest: Error processing background.persistent: Event pages are not currently supported. This will run as a persistent background page.
```


Expected results:

No warning messages should be printed when `background.persistent` is set to `true`, as persistent background pages are fully supported.

Updated

2 years ago
Keywords: good-first-bug
Priority: -- → P5
Whiteboard: triaged
Mentor: bob.silverberg

Comment 1

2 years ago
Hi, I am interested in fixing this one.
How do you suggest I should approach for the fix ? 
Where do the functions for this live ?
Flags: needinfo?(bob.silverberg)
Thanks for your interest in taking this bug on, Ahmed.

I am going to be away for the next couple of weeks, so I'm going to ask Shane if he can take over for me as mentor in my absence.

I took a quick look at the code and I'm not sure if the change is as simple as may have originally been thought, but I'll put my observations in here and maybe you and Shane can work out if it's still a good first bug.

The warning is being generated because the `persistent` property is marked as `deprecated` in manifest.json at [1] and [2] via the `PersistentBackgroundProperty` id [3].

The code that issues warnings for deprecated entries can be found in Schemas.jsm at [4], which is called by `checkDeprecated` [5].

This code is generic in that it will issue a warning for any deprecated entry, regardless of the value of that entry, so to fix this bug we will need to _not_ use `deprecated` for the `persistent` property. We could add some special-cased code into Schemas.jsm to deal with the `persistent` property and issue a warning if its value is not `true`, although I don't really see much special casing in there currently to deal with specific properties. Maybe a new type in manifest.json for something like `DeprecatedIfNotTrue` would work, but that seems pretty ugly.

Shane, would you be able to help Ahmed over the next couple of weeks with this bug, starting with a suggestion about how we can approach a fix?

[1] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/schemas/manifest.json#102-105
[2] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/schemas/manifest.json#116-119
[3] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/schemas/manifest.json#436-438
[4] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#1017-1039
[5] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#1041-1052
Flags: needinfo?(bob.silverberg) → needinfo?(mixedpuppy)
I'm wondering if a new type, deprecatedValues, would be appropriate.  A list of values should be allowed as the value for it.  Then the code Schemas.jsm pointed to in comment 2 can be modified to also check the values against this.

You probably need to look through Schemas.jsm[1] for where "deprecated" is defined and add similar code for "deprecatedValues".

After that, the schema could look like:

{
 "id": "PersistentBackgroundProperty",
 "type": "boolean",
 "deprecated": "Event pages are not currently supported. This will run as a persistent background page.",
 "deprecatedValues": ["false"]
}


[1] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#1080
    http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/Schemas.jsm#943-959

Let me know if you need any further guidance.
Flags: needinfo?(mixedpuppy) → needinfo?(3ugzilla)

Comment 4

2 years ago
Hi, can I take this up?

Comment 5

2 years ago
Yes please, I am a bit busy now.
For instructions, see comment 2 and comment 3
Flags: needinfo?(3ugzilla)

Comment 6

2 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> I'm wondering if a new type, deprecatedValues, would be appropriate.  A list
> of values should be allowed as the value for it.  Then the code Schemas.jsm
> pointed to in comment 2 can be modified to also check the values against
> this.
> 
> You probably need to look through Schemas.jsm[1] for where "deprecated" is
> defined and add similar code for "deprecatedValues".
> 
> After that, the schema could look like:
> 
> {
>  "id": "PersistentBackgroundProperty",
>  "type": "boolean",
>  "deprecated": "Event pages are not currently supported. This will run as a
> persistent background page.",
>  "deprecatedValues": ["false"]
> }
> 
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#1080
>    
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#943-959
> 
> Let me know if you need any further guidance.

So, I only have to make the changes in
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#1080 .  and   > http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#943-959 ?

Updated

2 years ago
Flags: needinfo?(bob.silverberg)
(In reply to apoorvasingh2811 from comment #7)
> ^ 
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#1080 and
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/
> Schemas.jsm#943-959 ?

That looks right, according to what Shane suggested, but I think you'll also want to change checkDeprecated at [1] as well to deal with the new deprecatedValues logic. Perhaps that can be avoided, so you'll want to experiment a bit to see what works best.

[1] http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/toolkit/components/extensions/Schemas.jsm#1079
Flags: needinfo?(bob.silverberg)
Mentor: bob.silverberg → rob

Updated

10 months ago
Product: Toolkit → WebExtensions
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General
(Reporter)

Updated

8 months ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All

Updated

7 months ago
Duplicate of this bug: 1492208
> The following warning message was printed into the console:
> ```
> addons.webextension.persistent-background-page@bmo-test-extension	WARN
> Loading extension 'persistent-background-page@bmo-test-extension': Reading
> manifest: Error processing background.persistent: Event pages are not
> currently supported. This will run as a persistent background page.
> ```

> 
> Expected results:
> 
> No warning messages should be printed when `background.persistent` is set to
> `true`, as persistent background pages are fully supported.

I think we actually do need to generate a warning for background.persistent=true, but because it doesn't affect the extension's ability to run, it shouldn't come across as an error message. 
 
The message text should be changed to read: Reading manifest: Warning: Event pages are not currently supported. This will run as a persistent background page."
If this is your first contribution to Firefox, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for how to get started. 

Please needinfo the bug's mentor if you have any questions.

Updated

7 months ago
Flags: needinfo?(mconca)
(In reply to Caitlin Neiman [:caitmuenster] from comment #11)
> The message text should be changed to read: Reading manifest: Warning: Event
> pages are not currently supported. This will run as a persistent background
> page.

I agree that 1) we should still surface a message until such time as we support event pages, and 2) changing this to a warning would make more sense.
Flags: needinfo?(mconca)

Updated

4 months ago
Duplicate of this bug: 1512378
(Assignee)

Comment 15

4 months ago
Hi I am completely new to contributing, but would like to give this a try.
(Assignee)

Comment 16

4 months ago
is there any extension with background persistence to use as a test? the one attached is unverified
Hi Edward, and welcome! You can download the attached example as a test (it's ok if it's unverified - you can see the error message when you temporarily install it through about:debugging). 

Please feel welcome to needinfo :robwu if you need any help!
Assignee: nobody → edward.i.wu
(Assignee)

Comment 18

4 months ago
(In reply to Caitlin Neiman [:caitmuenster] from comment #17)
> Hi Edward, and welcome! You can download the attached example as a test
> (it's ok if it's unverified - you can see the error message when you
> temporarily install it through about:debugging). 
> 
> Please feel welcome to needinfo :robwu if you need any help!

Thanks Caitlin :D
(Assignee)

Comment 19

4 months ago
Hi Rob,
In addition to the highlighted code sections above, I found the error and makeError functions of the context class of Schemas.jsm that print out the error text. However if the solution is the print a Warning(instead of "Error processing..."), where could that be done? Also do the discussions about adding a new deprecatedValue type still apply if we want a warning? Any tips on how to get started, or better understand the code would be very much appreciated.
Flags: needinfo?(rob)

Comment 20

4 months ago
"Error processing ..." is auto-generated at [1], and its purpose is to show the source of the error/warning.

Since the message is non-fatal (and doesn't prevent the extension from loading), I suggest to not bother with introducing a distinction between warning and error.


The previously suggested "defaultValues" property is a bit limited, because there is no way to associate a deprecation message with a value.

So instead of introducing a new "defaultValues" property, I recommend re-using the existing "choices" property. The "choices" property in the schema can be used to define an union of types. We want to be able to define a boolean type with value true (without deprecation message) and another boolean type with value false (with the existing deprecation message).
Currently, there is no way to define a boolean type with a specific value, so this is where you need to introduce new logic to Schemas.jsm

The existing schema format already has a concept for limiting the number of values: String types can have an "enum" property to limit to a specific range of choices. So let's introduce this to the boolean type too, so that the definition of "persistent" (at [2]) can be changed to something like this:

      {
        "id": "PersistentBackgroundProperty",
        "choices": [
          {
            "type": "boolean",
            "enum": [true]
          },
          {
            "type": "boolean",
            "enum": [false],
            "deprecated": "Event pages are not currently supported. This will run as a persistent background page."
          }
        ]
      }

> Any tips on how to get started, or better understand the code would be very much appreciated.

If you haven't read it before, I suggest to start with reading https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Then, once you are somewhat familiar with the development process, use search for "enum" in Schemas.jsm to see how StringType supports enumerations. The actual implementation for booleans (to be added to the existing BooleanType class) is much simpler, and you can probably figure it out.

While you can try to re-compile Firefox and do manual testing, it is probably much faster to write a unit test and run that test to see if your code works as expected. So start off by changing Schemas.jsm and adding a unit test to toolkit/components/extensions/test/xpcshell/test_ext_schemas.js (this test should verify that "enum" is supported in BooleanType).

Once that works, you can update the schema for PersistentBackgroundProperty as shown above (at [2]), and add a new test to toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js to confirm that no message is displayed [4].

Once that works, the patch is ready for review (and just to be sure you can try to manually test your feature by loading the extension from comment 0 at about:debugging and confirming that no error message appears).

[1] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/Schemas.jsm#431
[2] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/schemas/manifest.json#579-583
[3] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
[4] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js#63
Flags: needinfo?(rob)
(Assignee)

Comment 21

4 months ago
(In reply to Rob Wu [:robwu] from comment #20)
> "Error processing ..." is auto-generated at [1], and its purpose is to show
> the source of the error/warning.
> 
> Since the message is non-fatal (and doesn't prevent the extension from
> loading), I suggest to not bother with introducing a distinction between
> warning and error.
> 
> 
> The previously suggested "defaultValues" property is a bit limited, because
> there is no way to associate a deprecation message with a value.
> 
> So instead of introducing a new "defaultValues" property, I recommend
> re-using the existing "choices" property. The "choices" property in the
> schema can be used to define an union of types. We want to be able to define
> a boolean type with value true (without deprecation message) and another
> boolean type with value false (with the existing deprecation message).
> Currently, there is no way to define a boolean type with a specific value,
> so this is where you need to introduce new logic to Schemas.jsm
> 
> The existing schema format already has a concept for limiting the number of
> values: String types can have an "enum" property to limit to a specific
> range of choices. So let's introduce this to the boolean type too, so that
> the definition of "persistent" (at [2]) can be changed to something like
> this:
> 
>       {
>         "id": "PersistentBackgroundProperty",
>         "choices": [
>           {
>             "type": "boolean",
>             "enum": [true]
>           },
>           {
>             "type": "boolean",
>             "enum": [false],
>             "deprecated": "Event pages are not currently supported. This
> will run as a persistent background page."
>           }
>         ]
>       }
> 
> > Any tips on how to get started, or better understand the code would be very much appreciated.
> 
> If you haven't read it before, I suggest to start with reading
> https://wiki.mozilla.org/WebExtensions/Contribution_Onramp
> 
> Then, once you are somewhat familiar with the development process, use
> search for "enum" in Schemas.jsm to see how StringType supports
> enumerations. The actual implementation for booleans (to be added to the
> existing BooleanType class) is much simpler, and you can probably figure it
> out.
> 
> While you can try to re-compile Firefox and do manual testing, it is
> probably much faster to write a unit test and run that test to see if your
> code works as expected. So start off by changing Schemas.jsm and adding a
> unit test to toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
> (this test should verify that "enum" is supported in BooleanType).
> 
> Once that works, you can update the schema for PersistentBackgroundProperty
> as shown above (at [2]), and add a new test to
> toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js to
> confirm that no message is displayed [4].
> 
> Once that works, the patch is ready for review (and just to be sure you can
> try to manually test your feature by loading the extension from comment 0 at
> about:debugging and confirming that no error message appears).
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/
> Schemas.jsm#431
> [2]
> https://searchfox.org/mozilla-central/rev/
> 232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/
> schemas/manifest.json#579-583
> [3]
> https://searchfox.org/mozilla-central/rev/
> 232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/
> xpcshell/test_ext_schemas.js
> [4]
> https://searchfox.org/mozilla-central/rev/
> 232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/
> xpcshell/test_ext_eventpage_warning.js#63

Thank you Rob, I'll try to digest all this and let you know if I have any questions
(Assignee)

Comment 22

4 months ago
I've made the changes to Schema.jsm and added the changes you wrote to manifest.json, which removes the warning when adding the example extension in about:debugging manually, but I am having trouble adding unit tests to text_ext_schemas.js. 

Trying to add just a equals test, similar to how they do at: 

https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js#522-524

but the json type is not processing the way I think it would. 


       let booleanEnumJson = [{
          namespace: "booleanEnum",

          types: [
            {
             "id": "enumTrue",
             "type": "boolean",
             "enum": [true],
            },
            {
             "id": "enumFalse",
             "type": "boolean",
             "enum": [false],
            },
        ],
      }];


using

 ok(root.booleanEnum, "namespace exists");

in an add_task function shows that the json is getting loaded as:

       {"enumTrue":"undefined","enumFalse":"undefined"}

The same happens if I remove enum, and if I add a default value of true. 
Is this the right way to approach the test? Which pieces am I missing?
(Assignee)

Updated

4 months ago
Flags: needinfo?(rob)

Comment 23

4 months ago
Could you put up your patch to Schemas.jsm? It's difficult to tell what's going on without seeing your changes.
Flags: needinfo?(rob)
(Assignee)

Comment 24

4 months ago
These were the changes to the BooleanType class: 


class BooleanType extends Type {
  static get EXTRA_PROPERTIES() {
    return ["enum", ...super.EXTRA_PROPERTIES];
  }

  static parseSchema(root, schema, path, extraProperties = []) {
    this.checkSchemaProperties(schema, path, extraProperties);
    let enumeration = schema.enum || null;
    return new this(schema, enumeration);
  }

  constructor(schema, enumeration) {
    super(schema);
    this.enumeration = enumeration;
  }

  normalize(value, context) {
    return this.normalizeBase("boolean", value, context);
  }

  checkBaseType(baseType) {
    return baseType == "boolean";
  }

  checkDeprecated(context, value = null) {
    if (this.deprecated && this.enumeration && !this.enumeration[0]) {
      this.logDeprecation(context, value);
    }
  }
}

Comment 25

4 months ago
Regarding comment 22: I think that you were expecting root.booleanEnum.enumTrue to be true, but it is actuall undefined. This is fine; we are merely using "enum" as a way to limit the range of values for a boolean. With StringType, you can see more involved logic in the getDescriptor method, but that only exists because the range of possible string values is large, so it makes sense to export the possible values of a (string) enumeration to the namespace.
A boolean type can only have two values, true and false. I don't think that it is necessary to have StringType's behavior for generating the descriptor (i.e. letting root.booleanEnum be {ENUMTRUE: true, ENUMFALSE: false}).
If you want to test the value at all, then I would verify that the value is undefined.

Your current change ensures that the enumeration (if any) is attached to the BooleanType, but it is not used anywhere.
As you can see at the existing StringType implementation [1], the `normalize` method is responsible for generating the expected value, and you need to add validation if needed.

To test that your code works, write a test where a function requires a parameter of the booleanEnum.enumTrue. The test should verify that calling the method with boolean true succeeds, and boolean false fails.


( The desired use case (from my comment 20) is using the boolean+enum in a "choices" array. In that case, the BooleanType's normalize method will be called by the ChoiceType's normalize method [2]. )


[1] https://searchfox.org/mozilla-central/rev/70a4778dd84ec94b4b3a2537fd28482de45b9a00/toolkit/components/extensions/Schemas.jsm#1470-1486
[2] https://searchfox.org/mozilla-central/rev/70a4778dd84ec94b4b3a2537fd28482de45b9a00/toolkit/components/extensions/Schemas.jsm#1333
(Assignee)

Comment 26

4 months ago
Ah ok after rereading your comments, I think I understand now. Usually the value parameter in the BooleanType normalize function is undefined(because there is no way to define a boolean type with a specific value), and but if there exists an enum, we want to "transfer" the enum choice to the value variable in the normalize function and return either {value: true} or {value:false}?

Comment 27

4 months ago
(In reply to edward.i.wu from comment #26)
> Ah ok after rereading your comments, I think I understand now. Usually the
> value parameter in the BooleanType normalize function is undefined(because
> there is no way to define a boolean type with a specific value), and but if
> there exists an enum, we want to "transfer" the enum choice to the value
> variable in the normalize function and return either {value: true} or
> {value:false}?

"normalize" doesn't "transfer" the enum choice; "normalize" uses "enum" while processing the input value: When an enum is present, normalize should only return the input value (return {value}) if it is in the enumeration, and otherwise return an error.
(Assignee)

Comment 28

4 months ago
I've wrote normalize and have base tests that pass in text_ext_schemas.js that verify for the working case, and checks for an exact error of: 

      /Type error for parameter arg \(Invalid enumeration value false\) for booleanEnum\.paramMustBeTrue\./

in the error case. 

I will look at test_ext_eventpage_warning.js tomorrow, and add a choices test case as well.
(Assignee)

Comment 29

4 months ago
Added cases in test_ext_eventpage_warning.js with persistent = true, and checking console messages it generates against an empty string. Now I am setting up phabricator and arcanist to submit the patch.
(Assignee)

Comment 30

4 months ago
After pulling the latest code and merging I can't run 

./mach test toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js

without it skipping all the tests. It says MOZ_NODE_PATH env variable is not set, but after I set it in windows system variables as variable=MOZ_NODE_PATH and value=C:\Program Files\nodejs i get this other error: 


$ ./mach test toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js
 0:43.12 ERROR node not found at MOZ_NODE_PATH C:\Program Files\nodejs
Error running mach:

    ['test', 'toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

IOError: node not found at MOZ_NODE_PATH C:\Program Files\nodejs

  File "c:\mozilla-source\mozilla-central\testing/mach_commands.py", line 169, in test
    argv=extra_args, test_objects=tests, **kwargs)
  File "c:\mozilla-source\mozilla-central\python/mach\mach\registrar.py", line 138, in dispatch
    return self._run_command_handler(handler, context=context, **kwargs)
  File "c:\mozilla-source\mozilla-central\python/mach\mach\registrar.py", line 95, in _run_command_handler
    result = fn(**kwargs)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 260, in run_xpcshell_test
    return xpcshell.run_test(**params)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 60, in run_test
    return self.run_suite(**kwargs)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 46, in run_suite
    return self._run_xpcshell_harness(**kwargs)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 136, in _run_xpcshell_harness
    result = xpcshell.runTests(filtered_args)
  File "c:\mozilla-source\mozilla-central\testing/xpcshell\runxpcshelltests.py", line 1322, in runTests
    self.trySetupNode()
  File "c:\mozilla-source\mozilla-central\testing/xpcshell\runxpcshelltests.py", line 1109, in trySetupNode
    raise IOError(error)
Flags: needinfo?(rob)

Comment 31

4 months ago
MOZ_NODE_PATH is expected to be the path to the binary (.exe file), not the directory.
Flags: needinfo?(rob)
(Assignee)

Comment 32

4 months ago
Ah thank you, now it finds node. However, the tests are still being skipped in test_ext_eventpage_warning.js, even when I revert to the remote code base, without any of my local changes. There's no error, it just says:

 0:06.65 TEST_START: xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js
 0:06.65 TEST_START: xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js
 0:06.65 TEST_END: SKIP
 0:06.65 TEST_END: SKIP

At the bottom summary, it states: 

Ran 2 checks (2 tests)
Expected results: 0
Skipped: 2 tests


How should I get these tests to run? Or is it ok to submit the patch with only test_ext_schemas.js running?

Comment 33

4 months ago
"mach test" currently behaves unexpectedly because of bug 1498636.

Use "mach xpcshell-test" instead, to launch the tests.

(FWIW, I think that you didn't even have to set MOZ_NODE_PATH because the WebExtensions tests are not using it)
(Assignee)

Comment 34

4 months ago
prevent error msgs for background.persistent = true by allowing BooleanType in Schema.jsm to accept enums and PersistentBackgroundProperty in
manifest.json to contain 2 separate choices with a single possible enum each.
(Assignee)

Comment 35

4 months ago
Thanks Rob, that let me run and fix up the eventpage warning tests to submit the patch above.
Attachment #9033736 - Attachment description: Bug 1370077 prevent error msgs for background.persistent = true by allowing BooleanType in Schema.jsm to accept enums and PersistentBackgroundProperty inmanifest.json to contain 2 separate choices with a single possible enum each. r=dmose r=robwu → Bug 1370077 - Avoid deprecation message when background.persistent is true r=robwu
(Assignee)

Comment 36

4 months ago
Hi Rob, updated the patch, let me know what you think.
Flags: needinfo?(rob)

Updated

4 months ago
Flags: needinfo?(rob)
(Assignee)

Comment 37

3 months ago
Keywords: che
Keywords: checkin-needed
(Assignee)

Comment 38

3 months ago
The last patch is the same as the approved one before, but with an automated test detecting an error msg that I added changed to match the edits made to the error message during review.

Comment 39

3 months ago
Please wait for aswan's approval (r+) before adding checkin-needed.

As a mentor I am able to help you with guiding through the bug and the review, but all patches need to have be approved by a module peer (https://www.mozilla.org/en-US/about/governance/policies/module-owners) prior to landing, which I am not (but aswan is).
Keywords: checkin-needed
(Assignee)

Comment 40

3 months ago
Ok great, Thank you Rob for your patience and all the help. It was interesting learning the submission process and how to read and decipher a large code base like the extensions, good experience I hope.
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 41

3 months ago

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e676737b9689
Avoid deprecation message when background.persistent is true r=robwu,aswan

Keywords: checkin-needed

Comment 42

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Thanks so much for the patch, Edward! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Updated

3 months ago
Status: RESOLVED → VERIFIED

Comment 44

3 months ago

Verified using the attached test extension and I've managed to reproduce the issue on the reported version 53 and 55. On Nightly v66.0a1 (20190116093310) the console does not show the described error. Tests were run using windows 10 x64 bit, attaching screenshot

Comment 45

3 months ago
Posted image verified.png
Flags: in-testsuite+
(Assignee)

Comment 46

3 months ago

(In reply to Caitlin Neiman [:caitmuenster] from comment #43)

Thanks so much for the patch, Edward! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Thanks Caitlin! That'd be pretty cool, I've just created a mozillian account here:

https://mozillians.org/en-US/u/edward.i.wu/
Flags: needinfo?(cneiman)

Woohoo! You're vouched. \o/ I look forward to seeing you around the project!

Flags: needinfo?(cneiman)
(Assignee)

Comment 48

3 months ago

(In reply to Caitlin Neiman [:caitmuenster] from comment #47)

Woohoo! You're vouched. \o/ I look forward to seeing you around the project!\

Thanks!

You need to log in before you can comment on or make changes to this bug.