Extensions which specify this property with a value that we don't support are doing to wind up in an environment they don't expect, and may wind up causing private browsing data to leak. For the moment, we should continue to default to "spanning" mode, since we don't have any support for other modes, but explicitly specifying another mode should be an error.

> but explicitly specifying another mode should be an error. That's a little bit harsh, isn't it? I'd expect the extension authors then to change the value to "spanning" maybe without doing further changes, which isn't any better. Is it possible to "show" these extensions only non-incognito mode windows and tabs?

"possible", but that doesn't seem like a good first bug. This is a really quick and easy way to signal to developers what's going to happen and reduce the number of surprises for users and developers. Seems like a good idea to me, the work to continue on improving the modes can continue.

I agree. It would be possible to only show extensions non-incognito data, yes, but that would be the same as implementing `"incognito": "not_allowed"`, which is much more work than simply validating the property. For the moment, I'm more concerned about giving developers consistent behavior.

This should be implemented by adding an optional "incognito" property to the manifest schema[1]. It should be an enumerated string property, with one choice: "spanning" We should also add an xpcshell test[2] to check the validation. This test should serve as a good blueprint for that: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_manifest_content_security_policy.js [1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/manifest.json [2]: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

Linter updated for AMO: https://github.com/mozilla/web-extension-manifest-schema/pull/140

Kris, at the moment it looks like I have some time available this week to work on new bugs. Would you like me to take this one off your hands?

Sure, but this will also need a separate version of the patch with `"onError": "warn"` for uplift to Aurora and Beta.

Review commit: https://reviewboard.mozilla.org/r/58660/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/58660/

Comment on attachment 8761553 [details] [diff] [review] Version of the patch for uplift to Aurora and Beta Review of attachment 8761553 [details] [diff] [review]: ----------------------------------------------------------------- Let's land this version in m-c first, and create a separate bug to land the stricter version once it's baked for a couple of weeks.

For now we will land the version that generates an warning. I filed bug 1280027 as a follow-up to change the warning to an error, which we will land in the future.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/cf14f77f4f1d Issue a warning for manifests with any "incognito" property other than "spanning", r=kmag

This one is for uplift to aurora

(In reply to Bob Silverberg [:bsilverberg] from comment #15) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=16d8cc0fd731 Aurora try

(In reply to Bob Silverberg [:bsilverberg] from comment #18) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=68cfd16c2c21 beta try

Something seems to have gone horribly wrong with my pushes to try of the patch on top of aurora and beta. I'm trying again, but perhaps I'm not doing it right. The new push on top of beta is https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a5f2fcf1381. The new push on top of aurora is https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4440b7ba01e.

(In reply to Bob Silverberg [:bsilverberg] from comment #24) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cb853a07853 New aurora try

(In reply to Bob Silverberg [:bsilverberg] from comment #26) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7265ff440e79 New beta try

Comment on attachment 8763619 [details] [diff] [review] Patch for Aurora uplift Approval Request Comment [Feature/regressing bug #]: bug 1277686 [User impact if declined]: We would like to prevent extensions from using an unsupported value of "incognito". [Describe test coverage new/current, TreeHerder]: A new test was written to support this change, which is included in the patch. Treeherder results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cb853a07853 [Risks and why]: This is a very small change, with a test included, therefore can be considered low risk. [String/UUID change made/needed]: None.

(In reply to Bob Silverberg [:bsilverberg] from comment #28) > [Risks and why]: This is a very small change, with a test included, > therefore can be considered low risk. To add a bit more detail: This patch adds some metadata so that an unsupported setting in a WebExtension manifest triggers an error console warning, which should be visible to developers. In nightlies, unsupported values for this flag will soon trigger an error, rather than a warning, so uplifting this patch is likely to prevent add-ons from breaking as Firefox 50 rides the trains, as well as alerting developers that they won't be getting the behavior that they expect as a result of setting this flag.

Comment on attachment 8763619 [details] [diff] [review] Patch for Aurora uplift Approval Request Comment [Feature/regressing bug #]: bug 1277686 [User impact if declined]: We would like to prevent extensions from using an unsupported value of "incognito". [Describe test coverage new/current, TreeHerder]: A new test was written to support this change, which is included in the patch. Treeherder results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7265ff440e79 [Risks and why]: This is a very small change, with a test included, therefore can be considered low risk. [String/UUID change made/needed]: None. Also see Kris' additional comment above.

Comment on attachment 8763619 [details] [diff] [review] Patch for Aurora uplift OK, taking it to improve the webextension support taking it, should be in 48 beta 3

Verified fixed on Firefox 51.0a1 (2016-08-24), Firefox 50.0a2 (2016-08-25), Firefox 49 beta 6 (20160822111414) and Firefox 48.0.2 (20160823121617) under Windows 10 64-bit. The following warning is successfully thrown in Browser Console: “1472127171356 addons.webextension.EarthViewIcaTest@csrf.jp WARN Loading extension 'EarthViewIcaTest@csrf.jp': Reading manifest: Error processing incognito: Invalid enumeration value "split"”

I know its an old bug, but could we get a page about the incognito manifest property please?

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/incognito