Closed Bug 994782 Opened 6 years ago Closed 5 years ago

CSP in C++: remove old CSP backend (contentSecurityPolicy.js and CSPUtils.jsm) after deploying the new C++ backend.

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: geekboy, Assigned: geekboy)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

After we've enabled the new CSP parser and backend by default, we should remove the old implementation of nsIContentSecurityPolicy and all the things that use it (Contract ID is "@mozilla.org/contentsecuritypolicy;1").
Blocks: 994872
Blocks: 1011098
Attached patch remove_old_impl (obsolete) — Splinter Review
This is a first whack at removing the old backend.  Depending on test restructuring done in bugs 988616 and 993477, this will probably need to be updated before review and landing.
We're going to hold off on landing this until fx 33 has branched off to aurora.  I'll post a fresh patch now that bug 988616 and bug 925004 have landed.
Target Milestone: --- → mozilla34
Attached patch remove_old_impl (obsolete) — Splinter Review
Attachment #8431958 - Attachment is obsolete: true
Comment on attachment 8449788 [details] [diff] [review]
remove_old_impl

Even though we're going to wait to land this until Fx 34, lets get reviews started.  Garrett, Chris, can you take a look?
Attachment #8449788 - Flags: review?(mozilla)
Attachment #8449788 - Flags: review?(grobinson)
Comment on attachment 8449788 [details] [diff] [review]
remove_old_impl

Review of attachment 8449788 [details] [diff] [review]:
-----------------------------------------------------------------

Good start, but you missed some, have a look:
http://mxr.mozilla.org/mozilla-central/source/content/base/test/unit/test_csputils.js#501
http://mxr.mozilla.org/mozilla-central/source/content/base/test/unit/test_csp_ignores_path.js#9
or, better, grep for "@mozilla.org/contentsecuritypolicy;1".

http://mxr.mozilla.org/mozilla-central/source/content/base/test/unit/test_cspreports.js#10
also, grep for "CSPUtils.jsm"

B2G specific, http://mxr.mozilla.org/mozilla-central/source/b2g/installer/package-manifest.in#498
grep for: contentSecurityPolicy.js

Since this is patch is not going to land that soonish anyway, I am clearing the review flag for now.

::: content/base/test/csp/test_bug949549.html
@@ +18,1 @@
>                .createInstance(SpecialPowers.Ci.nsIContentSecurityPolicy);

Oh wow, this test was still using the old impl - good that we remove it.
Attachment #8449788 - Flags: review?(mozilla)
Attachment #8449788 - Flags: review?(grobinson)
I will remove test_csputils.js and test_csp_ignores_path.js since they're very CSPUtils.jsm-specific and we rolled those tests into the TestCSPParser.cpp tests.

Also, now that we're using a C++ component, the component registration isn't done via manifests, so we can remove contentSecurityPolicy.js from the various install manifests.
Depends on: 1034156
Attached patch remove_old_implSplinter Review
New patch, Chris.  Keeler: how do you feel about reviewing a code removal patch?  :)
Attachment #8449788 - Attachment is obsolete: true
Attachment #8455469 - Flags: review?(mozilla)
Attachment #8455469 - Flags: review?(dkeeler)
FWIW, this patch is applied on top of the patch for bug 1030936 in my queue, but it applies with some fuzz without that patch.  If you want you can apply the fast-path removal code first and then this one, but I don't think it matters.
Comment on attachment 8455469 [details] [diff] [review]
remove_old_impl

Review of attachment 8455469 [details] [diff] [review]:
-----------------------------------------------------------------

Your patch removes code? r=me.
But seriously, this is mostly code in content/, so this should probably get looked at by a DOM peer. (FWIW, I don't see any problems.)
Attachment #8455469 - Flags: review?(dkeeler)
Comment on attachment 8455469 [details] [diff] [review]
remove_old_impl

Review of attachment 8455469 [details] [diff] [review]:
-----------------------------------------------------------------

You addressed all of my concerns mentioned in comment 5. Please also run this through 'try' to make sure everything is peachy. Also, we have to wait till this can land till the 22nd, right? Overall looks great.

"Bye bye old CSP implementation - thanks for your help - but now it's time to let the strong C++ implementation handle the work - happy retirement :-)"
Attachment #8455469 - Flags: review?(mozilla) → review+
Thanks Chris.  
https://tbpl.mozilla.org/?tree=Try&rev=bc99b7aaabb7

We should probably get another DOM reviewer, what do you think?
Flags: needinfo?(mozilla)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #11)
> Thanks Chris.  
> https://tbpl.mozilla.org/?tree=Try&rev=bc99b7aaabb7
> 
> We should probably get another DOM reviewer, what do you think?

We should, I guess Johnny is currently out of the office, mabye smaug can take a look?
Flags: needinfo?(mozilla)
Comment on attachment 8455469 [details] [diff] [review]
remove_old_impl

jst: you're probably pretty backlogged, but if you feel like reviewing code removal, we need another DOM reviewer for this patch.

Also, pushed this along with bug 994872, bug 991468 and bug 991474 to try to see how they do:
https://tbpl.mozilla.org/?tree=Try&rev=23c1af15cb65
Attachment #8455469 - Flags: review?(jst)
Looks like there's a timeout test failure on B2G ICS Marionette-WebAPI tests, but I really doubt we're causing those.  Testing one more time with a fresh pull of m-c:
https://tbpl.mozilla.org/?tree=Try&rev=0032c07010e2
Yep, that was green.  Just waiting on r=jst at this point.
Comment on attachment 8455469 [details] [diff] [review]
remove_old_impl

*stamp*!
Attachment #8455469 - Flags: review?(jst) → review+
There was some minor bitrot in this patch when landing on mozilla-inbound, but it was a trivial merge to the package-manifest.in files.

https://hg.mozilla.org/integration/mozilla-inbound/rev/12f59ef39e41
https://hg.mozilla.org/mozilla-central/rev/12f59ef39e41
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1047519
Blocks: 1047528
Blocks: 1047537
You need to log in before you can comment on or make changes to this bug.