1006671 - Remove unnecessary JSContext argument to nsContentUtils::GetContentSecurityPolicy

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

See bug 997987 comment 12 and comment 13.

Comment 1

[Michael A. Milazzo]

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?

Comment 2

[Bobby Holley (:bholley)]

If the code compiles, it should be fine :-)

Comment 3

[Michael A. Milazzo]

Yeah, that's what I figured.  Hopefully I can get some time to work on it today.  Thanks!

Comment 4

[Nathan Yee [:nyee]]

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?

Comment 5

[Josh Matthews [:jdm]]

You are welcome to submit patches without being assigned. In general for newcomers we wait to assign them until there's a code submission.

Comment 6

[Nathan Yee [:nyee]]

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?

Comment 7

[Bobby Holley (:bholley)]

In this case compilation is fine. :-)

Comment 8

[Nathan Yee [:nyee]]

Attachment: Bug 1006671 - Remove JSContext argument in nsScriptSecurityManager::ReportError()

This is a partial patch; full bugfix WIP.

Comment 9

[Nathan Yee [:nyee]]

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 10

[Bobby Holley (:bholley)]

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!

Comment 11

[Nathan Yee [:nyee]]

Attachment: fix-bug-1006671

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8454206 [details] [diff] [review]
fix-bug-1006671

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

Looks great! r=me :-)

Comment 13

[Bobby Holley (:bholley)]

Pushing this to the tryserver. The only real danger here is compilation bustage.

https://tbpl.mozilla.org/?tree=Try&rev=08fadde88ab6

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4769de1b792b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4769de1b792b