• tschuster retitled this revision from WIP: Bug 2074026 - Always handle CSP before MixedContentBlocker to Bug 2074026 - Always handle CSP before MixedContentBlocker. r?emilio.

• tschuster added a reviewer: emilio.

Herald added a reviewer: layout-reviewers. · View Herald TranscriptWed, Sep 23, 2:49 PM

Herald added a subscriber: zrhoffman. · View Herald Transcript

I am totally open to alternative suggestions here. I was considering sorting the cached array for example, but that seemed a bit more difficult?

emilio added inline comments.Wed, Sep 23, 2:59 PM

dom/base/nsContentPolicy.cpp
48	I think you could `MOZ_ASSERT(NS_SUCCEDED(rv))` etc but...
124	So this works / doesn't do duplicate work because you removed the `content-policy` category entries right?

This seems testable perhaps? Also I have a vague memory of CSP and mixed content upgrade interacting poorly in the past, does that ring a bell? I couldn't find it immediately. I don't think this patch makes that any worse tho.

This revision is now accepted and ready to land.Wed, Sep 23, 3:04 PM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

Harbormaster completed remote builds in B1050801: Diff 1386502.Wed, Sep 23, 3:21 PM

• tschuster updated this revision to Diff 1387542.Thu, Sep 24, 9:25 AM

Yes, this definitely should have a test. Our previous "test coverage" here was accidental and was nullified by D327776.

dom/base/nsContentPolicy.cpp
124	Yes, exactly. It doesn't seem like we really query/use the `content-policy`, so it seems like removing it from the list would be the easiest to prevent both policies from being used twice.

The analysis task source-test-mozlint-rejected-words failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-c-includes failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-header-guards failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-file-whitespace failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-mscom-init failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-dot-mozilla-reference failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-test-manifest failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-license failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-clang-format failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-mingw-cap failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-file-perm failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-eslint failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-codespell failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-mozlint-trojan-source failed, but we could not detect any defect.
Please check this task manually.

If you see a problem in this automated review, please report it here.

Harbormaster failed remote builds in B1051534: Diff 1387542!Thu, Sep 24, 10:03 AM

This revision was landed with ongoing or failed builds.Thu, Sep 24, 2:26 PM

Closed by commit rFIREFOXAUTOLAND2a9780916b33: Bug 2074026 - Always handle CSP before MixedContentBlocker. r=emilio,layout… (authored by • tschuster). · Explain Why

This revision was automatically updated to reflect the committed changes.

• tschuster added a commit: rFIREFOXAUTOLAND2a9780916b33: Bug 2074026 - Always handle CSP before MixedContentBlocker. r=emilio,layout….

smolnar reopened this revision.Thu, Sep 24, 4:02 PM

This revision is now accepted and ready to land.Thu, Sep 24, 4:02 PM

smolnar added a reverting change: rFIREFOXAUTOLAND71357c09faf2: Revert "Bug 2074026 - Remove legacy_constructor for nsContentPolicy. r=emilio….Thu, Sep 24, 4:02 PM

• tschuster updated this revision to Diff 1389232.Fri, Sep 25, 3:00 PM

• tschuster added a reviewer: nalexander.Fri, Sep 25, 3:20 PM

This revision now requires review to proceed.Fri, Sep 25, 3:20 PM

Code analysis found 1 defect in diff 1389232:

1 defect found by test-manifest (Mozlint)

WARNING: Found 1 defect (warning level) that can be dismissed.

You can run this analysis locally with:

./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 1389232.

Harbormaster completed remote builds in B1052726: Diff 1389232.Fri, Sep 25, 3:27 PM

Fine by me.

This revision is now accepted and ready to land.Fri, Sep 25, 3:58 PM

Closed by commit rFIREFOXAUTOLAND93e47c6712e9: Bug 2074026 - Always handle CSP before MixedContentBlocker. r=emilio,layout… (authored by • tschuster). · Explain WhyFri, Sep 25, 4:33 PM

This revision was automatically updated to reflect the committed changes.

• tschuster added a commit: rFIREFOXAUTOLAND93e47c6712e9: Bug 2074026 - Always handle CSP before MixedContentBlocker. r=emilio,layout….

• tschuster mentioned this in rFIREFOXBETA2a9780916b33: Bug 2074026 - Always handle CSP before MixedContentBlocker. r=emilio,layout….Thu, Oct 8, 2:04 PM

• tschuster mentioned this in rFIREFOXBETA93e47c6712e9: Bug 2074026 - Always handle CSP before MixedContentBlocker. r=emilio,layout….Thu, Oct 8, 2:45 PM

Bug 2074026 - Always handle CSP before MixedContentBlocker. r?emilio
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1386502

dom/base/nsContentPolicy.h

dom/base/nsContentPolicy.cpp

layout/build/components.conf

Bug 2074026 - Always handle CSP before MixedContentBlocker. r?emilioClosedPublicActions