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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Sid Stamm [:geekboy or :sstamm]]

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").

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove_old_impl

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.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove_old_impl

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

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?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove_old_impl

New patch, Chris.  Keeler: how do you feel about reviewing a code removal patch?  :)

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

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 9

[Dana Keeler (she/her) [:keeler]]

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.)

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

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 :-)"

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Thanks Chris.  
https://tbpl.mozilla.org/?tree=Try&rev=bc99b7aaabb7

We should probably get another DOM reviewer, what do you think?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(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?

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Yep, that was green.  Just waiting on r=jst at this point.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 8455469 [details] [diff] [review]
remove_old_impl

*stamp*!

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12f59ef39e41