Closed Bug 1006671 Opened 6 years ago Closed 5 years ago

Remove unnecessary JSContext argument to nsContentUtils::GetContentSecurityPolicy

Categories

(Core :: Security: CAPS, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bholley, Assigned: nyee, Mentored)

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

Mentor: bobbyholley
Whiteboard: [mentor=bholley,lang=c++] → [lang=c++]
I'm interested in working on this.  Quick question though... if we're just removing JSContext* parameters, is there a set of unit tests that already covers the code that's being modified since it doesn't sound like behavior is changing?
Flags: needinfo?(bobbyholley)
If the code compiles, it should be fine :-)
Flags: needinfo?(bobbyholley)
Yeah, that's what I figured.  Hopefully I can get some time to work on it today.  Thanks!
I am currently working on this bug and would like to submit a patch. Do you have to be assigned to this bug to submit patches and if so, may I request to be assigned to this bug?
You are welcome to submit patches without being assigned. In general for newcomers we wait to assign them until there's a code submission.
I've completed my initial changes and confirmed it compiled and ran successfully. Should I test my changes before attaching the patch or is successful compilation good enough?
In this case compilation is fine. :-)
This is a partial patch; full bugfix WIP.
Attachment #8453578 - Flags: review?(bobbyholley)
I just realized that the function I changed was not a part of the comments in the Description. Should I revert my changes and should we only change the functions described in those comments?
Comment on attachment 8453578 [details] [diff] [review]
Bug 1006671 - Remove JSContext argument in nsScriptSecurityManager::ReportError()

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

::: caps/src/nsScriptSecurityManager.cpp
@@ -794,5 @@
> -            do_GetService("@mozilla.org/consoleservice;1"));
> -        NS_ENSURE_TRUE(console, NS_ERROR_FAILURE);
> -
> -        console->LogStringMessage(message.get());
> -    }

This part indicates that the JSContext argument is still being used and shouldn't be removed.

I just realized that this bug was kind of confusingly filed. The links in comment 0 discuss two functions: nsContentUtils::GetContentSecurityPolicy, and nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction. The former isn't on nsScriptSecurityManager, and the latter actually does make use of its JSContext argument (in the call to DescribeScriptedCaller).

So let's clarify this bug to be about nsContentUtils::GetContentSecurityPolicy. Sorry for the confusion!
Attachment #8453578 - Flags: review?(bobbyholley) → review-
Summary: Remove unnecessary JSContext argument to various functions on nsScriptSecurityManager → Remove unnecessary JSContext argument to nsContentUtils::GetContentSecurityPolicy
Attached patch fix-bug-1006671Splinter Review
Attachment #8453578 - Attachment is obsolete: true
Attachment #8454206 - Flags: review?(bobbyholley)
Comment on attachment 8454206 [details] [diff] [review]
fix-bug-1006671

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

Looks great! r=me :-)
Attachment #8454206 - Flags: review?(bobbyholley) → review+
Pushing this to the tryserver. The only real danger here is compilation bustage.

https://tbpl.mozilla.org/?tree=Try&rev=08fadde88ab6
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4769de1b792b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.