Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562953002:
Part 5.2: Is policy list subsumed under subsuming policy? (Closed)
DescriptionPart 5.2: Is policy list subsumed under subsuming policy?
In this CL we incorporate MediaListDirective subsumption logic
into the main subsumption flow on CSPDirectiveLevel.
BUG=647588
Committed: https://crrev.com/573bf02ee895447c8b662e66e8bff8d4d1a14aad
Cr-Commit-Position: refs/heads/master@{#438486}
Patch Set 1 #
Total comments: 3
Patch Set 2 : HeapVector #
Total comments: 3
Patch Set 3 : Removing trace from MediaListDirective #
Total comments: 6
Patch Set 4 : More test cases, removing .get() on members #
Total comments: 3
Messages
Total messages: 25 (13 generated)
amalika@google.com changed reviewers: + jochen@chromium.org, mkwst@chromium.org, oilpan-reviews@chromium.org
https://codereview.chromium.org/2562953002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2562953002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1270: std::vector<MediaListDirective*> pluginTypesOther; oilpan-reviews@ maybe this should be HeapVector<Member<>> instead? and therefore `bool MediaListDirective::subsumes(const std::vector<MediaListDirective*>& other)`, too? https://codereview.chromium.org/2562953002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right): https://codereview.chromium.org/2562953002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:624: TEST_F(CSPDirectiveListTest, SubsumesPluginTypes) { Subsumption of pluginTypes has been tested in MediaListDirectiveTest. Purpose of this test is to check interactions in the presence of both: SourceListDirectives and MediaListDirectives.
Description was changed from ========== Part 5.2: Is policy list subsumed under subsuming policy? In this CL we incorporate MediaListDirective subsumption logic into the main subsumption flow on CSPDirectiveLevel. BUG=647688 ========== to ========== Part 5.2: Is policy list subsumed under subsuming policy? In this CL we incorporate MediaListDirective subsumption logic into the main subsumption flow on CSPDirectiveLevel. BUG=647588 ==========
https://codereview.chromium.org/2562953002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2562953002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1270: std::vector<MediaListDirective*> pluginTypesOther; On 2016/12/09 at 10:24:40, amalika wrote: > oilpan-reviews@ maybe this should be HeapVector<Member<>> instead? > and therefore `bool MediaListDirective::subsumes(const std::vector<MediaListDirective*>& other)`, too? MediaListDirective isn't on the oilpan heap, so you can't use Member<> to reference it. However, in general we don't use STL containers in Blink, so I'd propose using WTF::Vector<MediaListDirective> or WTF::Vector<std::unique_ptr<MediaListDirective>> if you want to manage lifetime
Updated to using HeapVector of MediaListDirective members as per our discussion with jochen@ since m_pluginTypes is a `Member<MediaListDirective>` on CSPDirectiveList. https://codereview.chromium.org/2562953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/MediaListDirective.cpp (right): https://codereview.chromium.org/2562953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/MediaListDirective.cpp:121: CSPDirective::trace(visitor); This seems unnecessary since MediaListDirective does not have any heap members and CSPDirectiveList traces MediaListDirective so it will be released, but I added as per our discussion! https://codereview.chromium.org/2562953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp (left): https://codereview.chromium.org/2562953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:138: EXPECT_TRUE(emptyA.subsumes({&emptyA})); This is unnecessary since the first test cases tests the same.
haraken@chromium.org changed reviewers: + haraken@chromium.org
Oilpan part looks good. https://codereview.chromium.org/2562953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/MediaListDirective.cpp (right): https://codereview.chromium.org/2562953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/MediaListDirective.cpp:121: CSPDirective::trace(visitor); On 2016/12/13 08:31:52, amalika wrote: > This seems unnecessary since MediaListDirective does not have any heap members > and CSPDirectiveList traces MediaListDirective so it will be released, but I > added as per our discussion! It won't be needed.
Removing unnecessary DEFINE_TRACE from MediaListDirective
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
just a style nit wrt get() usage; when dealing with GC pointer types, get() is almost always redundant. Oilpan type usage looks just fine though; thanks for following up on the std::vector<> usage. https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1267: if (!m_pluginTypes.get()) There is an implicit pointer conversion in place for Member<>, so you can avoid the explicit get() here. Or (!hasPluginTypes()) https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp (left): https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:138: EXPECT_TRUE(emptyA.subsumes({&emptyA})); The test for the empty reflexive case isn't worth having?
https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1276: return m_pluginTypes->subsumes(pluginTypesOther); This looks like it's checking the union of all the policies in |other|, rather than the intersection, which doesn't seem correct. That is, given `type/1 type/2 type/3` and `type/1`, `type/1` should subsume, but it doesn't look like it will. Is that accurate? Can you add some tests along those lines so we can verify the behavior? https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right): https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:658: {"script-src http://example.com", "script-srcs http://example.com"}, Is `script-srcs` intentional?
Patchset #4 (id:60001) has been deleted
Addressing all the comments. Thank you for reviews! https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1276: return m_pluginTypes->subsumes(pluginTypesOther); On 2016/12/13 at 13:54:47, Mike West (slow) wrote: > This looks like it's checking the union of all the policies in |other|, rather than the intersection, which doesn't seem correct. That is, given `type/1 type/2 type/3` and `type/1`, `type/1` should subsume, but it doesn't look like it will. Is that accurate? > > Can you add some tests along those lines so we can verify the behavior? As with `SourceListDirective`, `subsumes` method of MediaListDirective makes sure to call `getIntersect` inside. So this should not be finding a union. I will add tests below already added in SubsumesPluginTypes https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp (left): https://codereview.chromium.org/2562953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:138: EXPECT_TRUE(emptyA.subsumes({&emptyA})); On 2016/12/13 at 13:38:54, sof wrote: > The test for the empty reflexive case isn't worth having? Sorry for confusion. I added an explanatory comment on the last patch but didnt add to this one. This case has the same effect as the test on line 80 above (`{{""}, true, true}`) so this check is redundant. https://codereview.chromium.org/2562953002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2562953002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1272: if (policy->hasPluginTypes()) Changing to hasPluginTypes() https://codereview.chromium.org/2562953002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right): https://codereview.chromium.org/2562953002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:666: true}, This is a test case of the example given: for `type/1 type/2 type/3` and `type/1`, the effective result should be subsumed.
LGTM, thank you. https://codereview.chromium.org/2562953002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right): https://codereview.chromium.org/2562953002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:666: true}, On 2016/12/13 at 14:42:08, amalika wrote: > This is a test case of the example given: for `type/1 type/2 type/3` and `type/1`, the effective result should be subsumed. Great!
The CQ bit was checked by amalika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amalika@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481717073789780,
"parent_rev": "c896d6bbdbf260819ff087b749befbfc8fde344b", "commit_rev":
"b88b0f5677fc6f2614417fd4067445dc566d261e"}
Message was sent while issue was closed.
Description was changed from ========== Part 5.2: Is policy list subsumed under subsuming policy? In this CL we incorporate MediaListDirective subsumption logic into the main subsumption flow on CSPDirectiveLevel. BUG=647588 ========== to ========== Part 5.2: Is policy list subsumed under subsuming policy? In this CL we incorporate MediaListDirective subsumption logic into the main subsumption flow on CSPDirectiveLevel. BUG=647588 Review-Url: https://codereview.chromium.org/2562953002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Part 5.2: Is policy list subsumed under subsuming policy? In this CL we incorporate MediaListDirective subsumption logic into the main subsumption flow on CSPDirectiveLevel. BUG=647588 Review-Url: https://codereview.chromium.org/2562953002 ========== to ========== Part 5.2: Is policy list subsumed under subsuming policy? In this CL we incorporate MediaListDirective subsumption logic into the main subsumption flow on CSPDirectiveLevel. BUG=647588 Committed: https://crrev.com/573bf02ee895447c8b662e66e8bff8d4d1a14aad Cr-Commit-Position: refs/heads/master@{#438486} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/573bf02ee895447c8b662e66e8bff8d4d1a14aad Cr-Commit-Position: refs/heads/master@{#438486} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
