Closed
Bug 994782
Opened 11 years ago
Closed 11 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)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: geekboy, Assigned: geekboy)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
150.62 KB,
patch
|
ckerschb
:
review+
jst
:
review+
|
Details | Diff | Splinter Review |
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").
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8431958 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8449788 -
Flags: review?(grobinson)
Assignee | ||
Comment 6•11 years ago
|
||
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.
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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 10•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
Thanks Chris.
https://tbpl.mozilla.org/?tree=Try&rev=bc99b7aaabb7
We should probably get another DOM reviewer, what do you think?
Flags: needinfo?(mozilla)
Comment 12•11 years ago
|
||
(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)
Assignee | ||
Comment 13•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
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
Assignee | ||
Comment 15•11 years ago
|
||
Yep, that was green. Just waiting on r=jst at this point.
Comment 16•11 years ago
|
||
Comment on attachment 8455469 [details] [diff] [review]
remove_old_impl
*stamp*!
Attachment #8455469 -
Flags: review?(jst) → review+
Assignee | ||
Comment 17•11 years ago
|
||
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•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•