iclelland, is this bug still something that should be addressed? If so, is there any upcoming work where it makes sense to allow tackle this bug?

(prompted by the triage of potentially stale, lower-priority bugs)

I still think this is a bug to fix. I agree using a <set> is a good idea but there is I think some subtle issues with how we parse the policy (please take with a grain of salt since I am not familiar with the spec on parsing this):

To compare the lists we use a tie which takes into account |matches_all_origins|. So my concern here is assuming that |matches_all_origins| is true, should we even have/compare the url::Origin lists?


From here:
https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/feature_policy/feature_policy.cc?rcl=bfd30be326fc1a0b28131588730a635f1849ed0c&l=122

I see as soon as we observer the '*' the parsing ends. This suggests we should not care about other origins. However, the way it is wired we get two different set of origins in the following cases:


1- <iframe allow="feature-foo * https://test.com">
2- <iframe allow="feature-foo https://test.com *">

i.e., the former case has an empty origin list but the latter has one item in it.

So IIUC we should:
  1- Clear |origins| if * is seen,
  2- Convert std::vector<url::Origin> to set::set.

I don't mind taking this bug to familiarize myself with the feature_policy code.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5eaea8ea16560bf8e34e4b50424e8c12d6affd07

commit 5eaea8ea16560bf8e34e4b50424e8c12d6affd07
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Mon Nov 26 18:08:50 2018

Fix some minor feature policy parsing issues

This CL makes the following changes to the feature policy parsing code:

  1- ParsedFeaturePolicyDeclaration holds a sorted vector of unique
     |origins|.
  2- AllowList uses std::set instead of std::vector.
  3- When parsing for list of origins, in case of matching all origins
     (*), the current set of origins is cleared.
  4- When comparing ParsedFeaturePolicyDeclaration, if both
     declarations include '*' then the set of origins are not compared.

The noticeable outcome of the CL is that parsed policy will ignore
repeated origins and will be sorted. This would make the feature lookup
algorithm more efficient.

Bug:  710324 
Change-Id: I5c67ee2d6cff891304781bea0998e07739006a2e
Reviewed-on: https://chromium-review.googlesource.com/c/1161753
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610887}
[modify] https://crrev.com/5eaea8ea16560bf8e34e4b50424e8c12d6affd07/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/5eaea8ea16560bf8e34e4b50424e8c12d6affd07/third_party/blink/common/feature_policy/feature_policy.cc
[modify] https://crrev.com/5eaea8ea16560bf8e34e4b50424e8c12d6affd07/third_party/blink/public/common/feature_policy/feature_policy.h
[modify] https://crrev.com/5eaea8ea16560bf8e34e4b50424e8c12d6affd07/third_party/blink/renderer/core/feature_policy/feature_policy.cc
[modify] https://crrev.com/5eaea8ea16560bf8e34e4b50424e8c12d6affd07/third_party/blink/web_tests/external/wpt/feature-policy/feature-policy-header-policy-allowed-for-some.https.sub.html
[modify] https://crrev.com/5eaea8ea16560bf8e34e4b50424e8c12d6affd07/third_party/blink/web_tests/external/wpt/feature-policy/feature-policy-header-policy-declined.https.sub.html
[modify] https://crrev.com/5eaea8ea16560bf8e34e4b50424e8c12d6affd07/third_party/blink/web_tests/http/tests/feature-policy/policy_main_document.php

Marking as fixed per CL landed in comment #4.