ext-cookies.js has some duplication of logic. To improve maintainability, the logic should be consolidated. In bug 1669716 I am introducing a new method to derive origin attributes from the received details, that method can be expanded to resolve this bug.

Duplicate logic (only difference is to return early vs throwing error):

• query: https://searchfox.org/mozilla-central/rev/f953948db2ac26698487f5fc5c03046c29922f26/toolkit/components/extensions/parent/ext-cookies.js#188-218,224-230
• set: https://searchfox.org/mozilla-central/rev/f953948db2ac26698487f5fc5c03046c29922f26/toolkit/components/extensions/parent/ext-cookies.js#417-440,455-459

Redundant private browsing check:

• remove: https://searchfox.org/mozilla-central/rev/f953948db2ac26698487f5fc5c03046c29922f26/toolkit/components/extensions/parent/ext-cookies.js#496-501
  • The check is redundant because query does already respect the private browsing check (and doesn't return private cookies if not allowed).

Changing severity to N/A because this is a task.

The cookies API implementation emits a bunch of errors in some cases,
but these are not covered by unit tests. I'm going to refactor some
logic involving these checks, and before doing so I'd like to start
with unit tests with the current behavior as expectations.

Move the duplicate logic of parsing the storeId to a common place.

This patch has the following observable changes in behavior:

• Invalid storeId (whether with the "firefox-container-" prefix or with
  something else) have the same error message instead of distinct ones.
  Instead of "Illegal storeId: Xor "Unknown storeId", the message is nowInvalid cookie store id: "X"`, which is also what Chromium uses.

• Invalid storeId in remove, get and getAll now result in an error,
  instead of being ignored silently. This is now consistent with the
  existing behavior for the cookies.set method, and also consistent with
  how Chromium rejects invalid storeId values.

The query method already validates whether access to private cookies is
allowed. It cannot yield private cookies if not allowed, so checking it
again in the loop of getAll is redundant..

Backed out for causing Android xpcshell failures on test_ext_cookies_errors.js.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=a219f44c01351e1604c587d3ee77ebc69560006b&selectedTaskRun=LQkwu-YcQcCMQlPTNSIJBA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=359704751&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/9fc1ffd42417840e9b9b6746eabcaacd6fef229b

Filed bug 1743616, updated test and will reland.

https://hg.mozilla.org/mozilla-central/rev/8a37f510e7d4
https://hg.mozilla.org/mozilla-central/rev/1a7011d80f7a
https://hg.mozilla.org/mozilla-central/rev/cfa39c8b2830