951457 - Create C++ CSP Parser and policy classes

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Sid Stamm [:geekboy or :sstamm]]

Before we can replace all of the JS used by CSP, we have to write a new C++ standalone parser and policy classes to represent policies.  These can exist alone with compiled code unit tests.  This is the equivalent of making a C++ version of CSPUtils.jsm and test_csputils.js.

The patches for this bug do nothing until bug 925004 ties it in.

Comment 1

[Garrett Robinson [:grobinson]]

Outline of landing plan:

0. (this bug) Land the new parser side by side with the old parser, behind a pref.

* maintain all of the tests for the old parser, but separate (or make them easy to separate them) from the 1.0+ tests (bug 988616)

The old parser will still parse everything (X- and 1.0+) in the product. The new parser tests, and any other new tests, will be running on TBPL.

We can also convert the existing Mochitests to test the spec-compliant cases with both the old parser and adding a stanza to flip the pref and run the same test with the new parser as well. These test changes should either land with the new parser, or shortly afterwards from a separate bug (not filed).

1. (Bug 925004) Pref on the new parser

* parse CSP 1.0+ headers with the new parser, X-headers with the old parser

We'll let this bake for a little while, and may do a release with both parsers in the product. This will allow us to shake out any bugs in the new implementation, and provides an easy fallback in case any serious issues pop up.

2. (Bug 949533) Remove the old parser, support for the X-header, and supporting tests/documentation once the new parser is stable.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_0_parser_utils.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_1_backend_stubs.patch

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_2_contentpolicy_tiein.patch

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_3_policycache.patch

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_4_new_backend_mochitests.patch

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_5_permitsAncestry.patch

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_6_parser_tests.patch

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_7_violatenreporting.patch

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Just uploaded our patches ready for review. TBPL looks good [1]. The big TODO for now, but shouldn't keep us from reviewing the patches, is to eliminate the memory leak problem. Problem this fix will go into part7_violationreporting.

[1] https://tbpl.mozilla.org/?tree=Try&rev=41ae8f4f0cbf

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8404134 [details] [diff] [review]
part_0_parser_utils.patch

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

Ready?

This parser looks really nice.  I'm going to be super picky because this is brand new, don't take it as criticism, this is a fantastic start!

How does the parser handle errors?  In most of gecko it relies on an nsresult return value (fake exceptions) and NS_ENSURE checks to propogate errors up the call stack.  With the way the parser is structured, it might make sense to keep an error code var for the internal error state and check it after calling complicated subroutines.... unless you have better ideas.

::: content/base/src/nsCSPParser.cpp
@@ +33,5 @@
> +  FRAME_SRC,
> +  FONT_SRC,
> +  CONNECT_SRC,
> +  REPORT_URI,
> +};

Can these be enums instead of strings?  If we use #define to make strings, use of the defined symbol in many places bloats the binary.

@@ +98,5 @@
> +#else
> +#define DUMP_POLICY(aPolicyString, aSelfURI, aReportOnly)
> +#define DUMP_FUNC(aFuncName)
> +#define DUMP_ERROR(aMsg, aDirective, aToken, aValue, aChar)
> +#endif

Please convert the DUMP/printf things into PR_LOG using the CSP module.

@@ +116,5 @@
> +nsCSPParser::~nsCSPParser()
> +{
> +}
> +
> +bool isCharacterToken(char16_t aSymbol)

Please put the return type on its own line (consistently, you do this elsewhere in the file).

@@ +123,5 @@
> +      (aSymbol >= 'A' && aSymbol <= 'Z')) {
> +    return true;
> +  }
> +  return false;
> +}

if (x) { return true; } return false;
  should be rewritten as:
return x;

@@ +137,5 @@
> +bool isDirective(const nsAString& aDir)
> +{
> +  uint32_t length = sizeof(DIRECTIVES) / sizeof(DIRECTIVES[0]);
> +
> +  for (uint32_t i = 0; i < length; i++) {

Can you just reuse length here?  for (; length > 0; length--) {...}

@@ +145,5 @@
> +  }
> +  return false;
> +}
> +
> +bool isQuoteLessKeyword(const nsAString& aKey) {

Nit: "Less" should not be capitalized, Quoteless is one word.

@@ +154,5 @@
> +  nsString keyword;
> +
> +  for (uint32_t i = 0; i < length; i++) {
> +    keyword.AssignASCII(KEYWORDS[i]);
> +    keyword.Trim("'");

Would it make more sense to store the keyword constants without quotes, or is it just as cumbersome to add them back?

@@ +176,5 @@
> +nsCSPPolicy*
> +nsCSPParser::parseContentSecurityPolicy(const nsAString& aPolicyString,
> +                                        nsIURI *aSelfURI,
> +                                        bool aReportOnly,
> +                                        uint64_t aInnerWindowID)

Please arrange the methods in this file so they are most specific to most general (method should rely on the ones above it and none below it).  This helps spacially compute dependencies and call chains when debugging or reading this file.

@@ +187,5 @@
> +                                        aSelfURI,
> +                                        aInnerWindowID);
> +
> +  // separate all input into tokens
> +  parser->generateTokens();

What happens if the token generator fails?  Do we just get incomplete internal state?

@@ +189,5 @@
> +
> +  // separate all input into tokens
> +  parser->generateTokens();
> +
> +  nsCSPPolicy *policy = parser->policy();

please add a comment to explain for what policy will be used.  Also nit: * goes with type.

@@ +195,5 @@
> +  if (aReportOnly) {
> +    policy->setReportOnlyFlag();
> +    nsString reportURI = NS_LITERAL_STRING(REPORT_URI);
> +    if (!policy->directiveExists(reportURI)) {
> +      nsXPIDLString reportOnlyHeader;

this nsXPIDLString isn't used, please remove.

@@ +197,5 @@
> +    nsString reportURI = NS_LITERAL_STRING(REPORT_URI);
> +    if (!policy->directiveExists(reportURI)) {
> +      nsXPIDLString reportOnlyHeader;
> +      nsAutoCString prePath;
> +      aSelfURI->GetPrePath(prePath);

Please check for failure in GetPrePath (check return val for NS_FAILED(rv)) and handle any failure.

@@ +212,5 @@
> +
> +void
> +nsCSPParser::generateNextToken()
> +{
> +  DUMP_FUNC("generateNextToken");

Crazy idea: can you rename DUMP_FUNC is something like LOG_PARSER_SUBROUTINE(functionName) so it's clearer what the purpose of this debugging stuff is?

@@ +225,5 @@
> +{
> +  DUMP_FUNC("generateTokens");
> +  nsTArray <nsString> dirAndSrcs;
> +
> +  while (true) {

while (true) scares me.  Can you put the end case in here?

while (mCurChar < mEndChar) {... }

@@ +246,5 @@
> +  if (*mCurChar == aSymbol) {
> +    advance();
> +    return true;
> +  }
> +  return false;

I see advance() used a lot with return true; where we could optimize the code a bit.  If advance returned a boolean instead of void, we could rewrite:
 if (foo) {
   advance();
   return true;
 }
 return false;
into:
 return foo && advance();

What do you think?

@@ +265,5 @@
> +{
> +  if (*mCurChar == aSymbol) {
> +    return true;
> +  }
> +  return false;

Please change everywhere:
  if(x) {return true;} return false;
to:
  return x;

@@ +284,5 @@
> +  mCurValue.Append(*mCurChar++);
> +}
> +
> +nsCSPPolicy*
> +nsCSPParser::policy()

Please put a comment before each parsing subroutine with its ABNF (this way it's obvious what it's parsing, and should map directly to the code within the subroutine).

@@ +309,5 @@
> +  const uint32_t NAME_INDEX = 0;
> +  nsString dirName = aTokens[NAME_INDEX];
> +  mCurDirName = dirName;
> +
> +  // if the directive already exists, we can ignore it

Please mention that this is defined in section 3.2.1.1 of the spec.

@@ +314,5 @@
> +  if (mPolicy->directiveExists(dirName)) {
> +    DUMP_ERROR(IGNORING_DIRECTIVE, mCurDirName, mCurToken, mCurValue, *mCurChar);
> +    const char16_t* params[] = { dirName.get() };
> +    logWarningErrorToConsole(nsIScriptError::warningFlag, "duplicateDirective",
> +                             params, ArrayLength(params));

Can you collapse the DUMP_ERROR and console logging into one macro (or two, one for "with console" and one for without)?  This cleans up error logging a bit throughout the file.

@@ +318,5 @@
> +                             params, ArrayLength(params));
> +    return;
> +  }
> +
> +  nsCSPDirective* cspDir = directiveName(dirName);

Since the caller is responsible for cleanup, would it make sense to require caller-allocated return values?  E.g., 

nsCSPDirective cspDir;  // on stack, auto-destroyed
void directiveName(dirName, &cspDir);

Or would that complicate things?  I think it would make it easier to do cleanup with multiple exit points in a function.

@@ +363,5 @@
> +                        nsTArray<nsCSPBaseSrc*>& outSrcs)
> +{
> +  DUMP_FUNC("sourceList");
> +  // at index 0 we store the name of the directive
> +  const uint32_t START_SRC_INDEX = 1;

I don't think you need this constant.  The comment is probably enough if you clarify that you mean the first *token* is always the directive name.

@@ +371,5 @@
> +    nsString value = aTokens[i];
> +
> +    // special case handling for none
> +    if (value.LowerCaseEqualsASCII(NONE)) {
> +      isNone = true;

What happens if the list contains 'none' and other tokens?  What does the spec say to do (do that here and explain it in a comment).

@@ +373,5 @@
> +    // special case handling for none
> +    if (value.LowerCaseEqualsASCII(NONE)) {
> +      isNone = true;
> +    }
> +    // special case handling for report-uri, which allows relative uris

So report-uri's value is not actually a source list, it's a list of URI references.  Perhaps it makes more sense to treat it differently and have a seperate parsing function for urireferenceList() ?

@@ +384,5 @@
> +      } else {
> +        const char16_t* params[] = { value.get() };
> +        logWarningErrorToConsole(nsIScriptError::warningFlag, "couldNotParseReportURI",
> +                                 params, ArrayLength(params));
> +      }

Since the URIs in report-uri value are URIs and not sources, please use the nsIURI parser to parse them (you can pass in selfURI to parse relative URI strings).

@@ +415,5 @@
> +  }
> +
> +  // 2) check if it is a nonce-source
> +  if (nsCSPNonceSource* cspNonce = nonceSource(aExpression)) {
> +      return cspNonce;

Please be consistent with indentation.  2 spaces for this file, looks like.

@@ +437,5 @@
> +   // continue parsing a host
> +   cspScheme->toString(false, parsedScheme);
> +   parsedScheme.ReplaceSubstring(NS_LITERAL_STRING("://"),
> +                                 NS_LITERAL_STRING(""));
> +    delete cspScheme;

Indentation is wacky here, please fix.

@@ +459,5 @@
> +}
> +
> +// Helper function for nonce and hash
> +bool
> +startsWith(const nsAString& aString, const nsAString& aPrefix) {

Can you use the already-in-gecko function StringBeginsWith?  You can do it case-insensitively.  There are a bunch of other string goodies in that file...
http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsReadableUtils.h#363

@@ +482,5 @@
> +  expr.Trim("'");
> +
> +  int32_t dashIndex = expr.FindChar('-') + 1;
> +  if (dashIndex < 0) {
> +    return nullptr; 

Nit: trailing whitespace.  Please scan the whole file for it and remove it.

@@ +493,5 @@
> +nsCSPHashSource*
> +nsCSPParser::hashSource(const nsAString& aExpr)
> +{
> +  DUMP_FUNC("hashSource");
> +  if (!startsWith(aExpr, NS_LITERAL_STRING("'"))) {

should this use "'hash-" (like ::nonceSource()) or just "'"?

@@ +519,5 @@
> +nsCSPSchemeSrc*
> +nsCSPParser::schemeSource()
> +{
> +  DUMP_FUNC("schemeSource");
> +  return scheme();

Why is there a schemeSource() *and* a scheme() subroutine?

@@ +544,5 @@
> +    return cspHost;
> +  }
> +  fileAndArguments(cspHost);
> +  return cspHost;
> +}

hostSource() is nice... very easy to read.

@@ +550,5 @@
> +nsCSPBaseSrc*
> +nsCSPParser::keywordSource(const nsAString& aKeyword)
> +{
> +  DUMP_FUNC("keywordSource");
> +  if (aKeyword.LowerCaseEqualsASCII(SELF)) {

Can we make a macro/function to compare a string to keyword?  CSP_IS_KEYWORD(aKeyword, SELF), for example...

@@ +574,5 @@
> +  if (accept(COLON) &&
> +      accept(SLASH) &&
> +      accept(SLASH)) {
> +    return new nsCSPSchemeSrc(scheme);
> +  }

Do all schemes have "://"? scheme() must also work for "data:".

@@ +594,5 @@
> +  return true;
> +}
> +
> +nsCSPHostSrc*
> +nsCSPParser::host(const nsAString& aScheme)

This function is long: can you make it clearer what it's doing and for each of the if statements, explain what kind of input has been parsed when we get there?  In essence, what rule requires each !accept(DOT)?  What's the parsed state look like in these cases?  (Just need more comments documenting how it works).

@@ +601,5 @@
> +
> +  if (accept(WILDCARD)) {
> +    // might solely the wildcard
> +    if (peek(END)) {
> +      nsCSPHostSrc *cspHost = new nsCSPHostSrc(aScheme);

why aScheme here?  Why can't we just create an nsCSPHostSrc without a scheme and then put the scheme into it later?

@@ +681,5 @@
> +          accept(DASH));
> +}
> +
> +bool
> +nsCSPParser::port(nsCSPHostSrc* aCspHost)

Is there a way to rewrite the port function to not depend on a host?  Or do we always have aCspHost before parsing the port?

@@ +782,5 @@
> +  }
> +}
> +
> +nsCSPHostSrc*
> +nsCSPParser::createHostSrcFromSelfURI()

Can we make this more generic, createHostSrcFromURI(nsIURI* aURI) ?  Might be useful in more than one situation, and might be a utils function...

::: content/base/src/nsCSPParser.h
@@ +30,5 @@
> +//
> +// The first element of each array has to be a valid directive-name, otherwise we can
> +// ignore the remaining elements of the array. Also, if the
> +// directive already exists in the current policy, we can ignore
> +// the remaining elements of that array. (http://www.w3.org/TR/CSP/#parsing)

Nit: make this a block comment, doc style.

Could you also include a short example "how to use the parser"?  Make the entry points clear.

@@ +76,5 @@
> +    nsCSPHostSrc* host(const nsAString& aScheme);                 // "*" / [ "*." ] 1*host-char *( "." 1*host-char )
> +    bool hostChar();                                              // ALPHA / DIGIT / "-"
> +    bool port(nsCSPHostSrc* aCspHost);                            // ":" ( 1*DIGIT / "*" )
> +    bool path(nsCSPHostSrc* aCspHost);
> +    void fileAndArguments(nsCSPHostSrc* aCspHost);

This is really super-nitty, but we can make this more readable... Can you line up the first char of each function?  Example:

void            directive(con...
nsCSPDirective* directiveName(con...
void            directiveValue(con...

@@ +101,5 @@
> +
> +    void logWarningErrorToConsole(uint32_t aSeverityFlag,
> +                                 const char* aProperty,
> +                                 const char16_t* aParams[],
> +                                 uint32_t aParamsLength);

Nit: misaligned args need one more space.

@@ +109,5 @@
> +    nsString                       mCurValue;
> +    nsTArray< nsTArray<nsString> > mTokens;
> +    nsIURI*                        mSelfURI;
> +    nsCSPPolicy*                   mPolicy;
> +    uint64_t                       mInnerWindowID; /* used for console reporting */

Can you add a comment above that shows the relationship of these cursors with a sample state?  Something like:

"this is a string being parsed"
           ^1    ^2
1: mCurChar
2: mEndChar
mCurValue = "string"
mTokens = ["this", "is", "a"]

::: content/base/src/nsCSPUtils.cpp
@@ +34,5 @@
> +
> +  // TEMPORARY
> +  // note that the console service won't be started for cppunittests. If you
> +  // want debugging printed to the terminal, uncomment:
> +  //printf("%s\n", aMsg);

Nice.

@@ +40,5 @@
> +
> +void CSP_GetLocalizedStr(const char16_t* aName,
> +                         const char16_t** aParams,
> +                         uint32_t aLength,
> +                         char16_t** outResult)

Nit: return type on its own line, consistently please.

@@ +51,5 @@
> +                                      getter_AddRefs(keyStringBundle));
> +  }
> +
> +  if (!keyStringBundle) {
> +    return;

Would it make sense to also set *outResult to ""?

@@ +62,5 @@
> +}
> +
> +void CSP_LogStrMessage(const nsAString& aMsg) {
> +  nsCOMPtr<nsIConsoleService> console(
> +    do_GetService("@mozilla.org/consoleservice;1"));

Do this all on one line or use = instead of ().

@@ +65,5 @@
> +  nsCOMPtr<nsIConsoleService> console(
> +    do_GetService("@mozilla.org/consoleservice;1"));
> +
> +  if (!console)
> +    return;

Brace your if, please.

@@ +83,5 @@
> +  nsCOMPtr<nsIConsoleService> console(
> +    do_GetService(NS_CONSOLESERVICE_CONTRACTID));
> +
> +   nsCOMPtr<nsIScriptError> error
> +     (do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));

The previous two initializations are the same pattern but you use different wrapping syntax.  Please use one line for each or use =.

@@ +86,5 @@
> +   nsCOMPtr<nsIScriptError> error
> +     (do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));
> +
> +   if (!console || !error)
> +     return;

Nit: brace your if.

@@ +89,5 @@
> +   if (!console || !error)
> +     return;
> +
> +  nsString errorMsg = NS_ConvertUTF8toUTF16("Content Security Policy: ");
> +  errorMsg.Append(aMessage);

Can you just do errorMsg.Append(NS_LITERAL_STRING("Content Security Policy: "))?

@@ +106,5 @@
> +               aSourceLine, aLineNumber,
> +               aColumnNumber, aFlags,
> +               aCategory);
> +  }
> +  console->LogMessage(error);

Is it possible to reach this line and error is broken (not ready to be passed to LogMessage)?  If so, please check for errors before calling LogMessage.

@@ +150,5 @@
> +
> +bool
> +nsCSPSchemeSrc::allows(enum AllowsType aType, const nsAString& aStr) const
> +{
> +  return false;

I don't understand why a scheme src never allows anything...

@@ +157,5 @@
> +void
> +nsCSPSchemeSrc::toString(bool aAppendPath, nsAString& outStr) const
> +{
> +  outStr.Append(mScheme);
> +  outStr.AppendASCII("://");

// should only be appended if there is also a host.  Doesn't make a whole lot of sense for things like javascript: and data: URIs.

@@ +177,5 @@
> +bool
> +nsCSPHostSrc::permits(nsIURI* aUri) const
> +{
> +  // WILDCARD; always allow
> +  if (mHost.EqualsASCII("*")) {

Would it be easier to store a boolean flag in nsCSPHostSrc that says whether it is all or none? (Faster comparison)?

@@ +186,5 @@
> +  nsAutoCString scheme;
> +  aUri->GetScheme(scheme);
> +  if (!mScheme.IsEmpty()) {
> +    if (!mScheme.EqualsASCII(scheme.get())) {
> +      return false;

Can you just use aUri.SchemeIs(mScheme.get()) here?

@@ +190,5 @@
> +      return false;
> +    }
> +  }
> +
> +  NS_ASSERTION((!mHost.IsEmpty()), "host can not be the empty string");

Why not?  Because it must be a host, * or 'none'?

@@ +192,5 @@
> +  }
> +
> +  NS_ASSERTION((!mHost.IsEmpty()), "host can not be the empty string");
> +
> +  // HOST; probably adjust wildcard part of host

Adjust how?  By removing *?  Please clarify in the comment.

@@ +197,5 @@
> +  nsString mHostAdjusted, uriHostAdjusted;
> +  mHostAdjusted = mHost;
> +  nsAutoCString uriHost;
> +  aUri->GetHost(uriHost);
> +  CopyUTF8toUTF16(uriHost, uriHostAdjusted);

This seems overly complicated.  Can it be simplified?

@@ +215,5 @@
> +    uriHostAdjusted = Substring(uriHostAdjusted, index, (uriHost.Length() - index));
> +  }
> +
> +  if (!mHostAdjusted.Equals(uriHostAdjusted)) {
> +    return false;

What about *.b.com and x.y.b.com?  Are they a match?  If so, can you use a reverse substring search (EndsWith) and see if the leftovers are just *?

@@ +231,5 @@
> +
> +  // if mPort is empty, we have to compare default ports
> +  if (mPort.IsEmpty()) {
> +    int32_t tmpAdjustedPort;
> +    tmpAdjustedPort = NS_GetDefaultPort(NS_ConvertUTF16toUTF8(mScheme).get());

You only use tmpAdjustedPort once, can you get by without creating a variable?

@@ +252,5 @@
> +
> +bool
> +nsCSPHostSrc::allows(enum AllowsType aType, const nsAString &aStr) const
> +{
> +  return false;

Please explain why this is always false.

@@ +262,5 @@
> +  // append scheme
> +  outStr.Append(mScheme);
> +
> +  // append host
> +  outStr.AppendASCII("://");

This doesn't make sense for all schemes, or if there's no host.  Please make sure slashes aren't included for schemes like data: and javascript:

@@ +284,5 @@
> +{
> +  if (mHost.Length() > 0) {
> +    return true;
> +  }
> +  return false;

See comment from other file.  This function body should just be:
 return mHost.Length() > 0;

@@ +288,5 @@
> +  return false;
> +}
> +
> +void
> +nsCSPHostSrc::addHost(const nsAString &aHost)

Rename this to setHost.  nsCSPHostSrc is not a list.

@@ +296,5 @@
> +  mHost = host;
> +}
> +
> +void
> +nsCSPHostSrc::addPort(const nsAString &aPort)

Same as addHost: rename to setPort, this is not a list.

@@ +312,5 @@
> +  mPath.Append(path);
> +}
> +
> +void
> +nsCSPHostSrc::addFileAndArguments(const nsAString &aFile)

setFileAndArguments?

@@ +326,5 @@
> +{
> +  nsString keyword = PromiseFlatString(aKeyword);
> +  ToLowerCase(keyword);
> +  NS_ASSERTION(!keyword.LowerCaseEqualsASCII(SELF), 
> +                "'self' should have been replaced in the parser");

Nits: trailing whitespace and indent mismatch.

@@ +341,5 @@
> +  // unsafe-eval, unsafe-inline and unsafe-eval are handled in allows
> +  if (mKeyword.EqualsASCII(NONE)) {
> +    return false;
> +  }
> +  return false;

This function body is a no-op.  Just change it to return false (or fix).

@@ +353,5 @@
> +  }
> +  if (mKeyword.Equals(aStr)) {
> +     return true;
> +  }
> +  return false;

Please optimize.  This function can be rewritten as:
 return mKeyword.Equals(aStr) && aType != KEYWORD;

@@ +376,5 @@
> +
> +bool
> +nsCSPNonceSource::permits(nsIURI* aUri) const
> +{
> +  return false;

Why? Please comment.

@@ +389,5 @@
> +
> +  if (mNonce.Equals(aStr)) {
> +    return true;
> +  }
> +   return false;

See above note about keyword src.  Same thing here.

@@ +417,5 @@
> +
> +bool
> +nsCSPHashSource::permits(nsIURI* aUri) const
> +{
> +  return false;

why?  Please comment.

@@ +454,5 @@
> +    if (NS_ConvertUTF16toUTF8(mHash).Equals(hash)) {
> +      return true;
> +    }
> +  }
> +  return false;

You can minimize some of the if() statements with NS_ENSURE_SUCCESS:

rv = blah;
NS_ENSURE_SUCCESS(rv, false);

rv = blah2;
NS_ENSURE_SUCCESS(rv, false);

...

@@ +527,5 @@
> +  }
> +}
> +
> +void
> +nsCSPDirective::addSrcs(const nsTArray<nsCSPBaseSrc*>& aSrcs)

setSrcs

@@ +540,5 @@
> +  ToLowerCase(name);
> +  if (mName.Equals(name)) {
> +    return true;
> +  }
> +  return false;

return mName.Equals(name);

@@ +584,5 @@
> +nsCSPDirective::directiveNameEqualsContentType(nsContentPolicyType aContentType) const
> +{
> +  // make sure we do not check for the default src before any other sources
> +  if (isDefaultDirective())
> +    return false;

braces, please.

@@ +594,5 @@
> +
> +  if (aContentType == directiveNameToContentType(mName)) {
> +    return true;
> +  }
> +  return false;

return aContentType == directiveNameToContentType(mName);

@@ +602,5 @@
> +nsCSPDirective::isDefaultDirective() const {
> +  if (mName.EqualsASCII(DEFAULT_SRC)) {
> +    return true;
> +  }
> +  return false;

Do we ever use this outside nsCSPDirective?  If not we don't need the function, we can just use mName.EqualsASCII(DEFAULT_SRC);

@@ +608,5 @@
> +
> +void
> +nsCSPDirective::getDirectiveName(nsAString& outDirectiveName) const
> +{
> +  outDirectiveName.Append(mName);

It's possible outDirectiveName has value already, you should use Assign().

@@ +638,5 @@
> +nsCSPPolicy::permits(nsContentPolicyType aContentType,
> +                     nsIURI* aUri,
> +                     nsAString& outViolatedDirective) const
> +{
> +  NS_ASSERTION(aUri, "no uri");

Please clarify what's required in the assertion message.

@@ +660,5 @@
> +      if (mDirectives[i]->permits(aUri)) {
> +        return true;
> +      }
> +      mDirectives[i]->getDirectiveName(outViolatedDirective);
> +      return false;

turn these around:

if (!directive[i]->permits(aUri)) {
  set outViolatedDirective;
  return false;
}
return true;

@@ +671,5 @@
> +
> +bool
> +nsCSPPolicy::allows(nsContentPolicyType aContentType,
> +                    enum AllowsType aType,
> +                    const nsAString& aStr) const

allows and permits have a lot of code in common.  Is there a way to factor out the code into one function and just route in additional data to allows?  Maybe there's nothing clean, but worth thinking about.

@@ +713,5 @@
> +{
> +  uint32_t length = mDirectives.Length();
> +  for (uint32_t i = 0; i < length; ++i) {
> +    mDirectives[i]->toString(outStr);
> +    outStr.AppendASCII(";");

No trailing ; please, only append it if there are more directives.

@@ +734,5 @@
> +
> +void
> +nsCSPPolicy::setReportOnlyFlag()
> +{
> +  mReportOnly = true;

add a parameter so you can set it to false if you want.

::: content/base/src/nsCSPUtils.h
@@ +16,5 @@
> +#define SELF            "'self'"
> +#define UNSAFE_INLINE   "'unsafe-inline'"
> +#define UNSAFE_EVAL     "'unsafe-eval'"
> +#define NONE            "'none'"
> +  

Nit: trailing whitespace.

@@ +17,5 @@
> +#define UNSAFE_INLINE   "'unsafe-inline'"
> +#define UNSAFE_EVAL     "'unsafe-eval'"
> +#define NONE            "'none'"
> +  
> +// please add any directives to DIRECTIVES in nsCSPParser.cpp

Can we move DIRECTIVES into this file instead?

@@ +27,5 @@
> +#define MEDIA_SRC       "media-src"
> +#define FRAME_SRC       "frame-src"
> +#define FONT_SRC        "font-src"
> +#define CONNECT_SRC     "connect-src"
> +#define REPORT_URI      "report-uri"

Can we make these static vars somehow?  Maybe class contstants for a CSP namespace that envelops all of the CSP parsed types?  Defines are fine, but for strings it can bloat the binary (bad for B2G) if used frequently.

namespace CSP {
  static const char_16t* DEFAULT_SRC = "default-src";
  ...
}

Then referencing the constants outside of this file would be as easy as #including it and then using CSP::DEFAULT_SRC or something?

I know you had compile problems with static consts before, but maybe if we scope them it will be easier?

@@ +47,5 @@
> +//   if (!gCSPLog)
> +//     gCSPLog = PR_NewLogModule("CSP");
> +//   return gCSPLog;
> +// }
> +// #define CSP_PR_LOG(type, msg) PR_LOG(GetCSPPRLog(), type, msg)

This should be fixed (uncommented) or removed before landing.

@@ +56,5 @@
> +
> +/* == Logging ============================= */
> +
> +void
> +CSPLog(const char *aMsg);

All on one line for consistency with the rest of this .h file.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_2_contentpolicy_tiein.patch

Fixed reporting for GetAllowsNonce/Hash and put the includes in alphabetical order.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_2_contentpolicy_tiein.patch

Uups, uploaded the wrong patch! Here is the right one!

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8404135 [details] [diff] [review]
part_1_backend_stubs.patch

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

r=me with the changes mentioned here and grobinson's feedback.

Please update the commit message to include bug number and include some language about this being the new C++ CSP implementation.

::: content/base/src/nsCSPContext.cpp
@@ +26,5 @@
> +#include "nsIAsyncVerifyRedirectCallback.h"
> +#include "nsAsyncRedirectVerifyHelper.h"
> +#include "mozilla/Preferences.h"
> +#include "nsIScriptError.h"
> +#include "nsContentUtils.h"

Please alphabetize the includes... so long as it doesn't break stuff.

@@ +36,5 @@
> +                   nsIContentSecurityPolicy,
> +                   nsIChannelEventSink,
> +                   nsISerializable)
> +
> +static const char* CSP_VIOLATION_TOPIC = "csp-on-violate-policy";

Please either make this a #define in nsCSPContext.h or (better) make it a constant in nsIContentSecurityPolicy!
For example: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIGlobalHistory2.idl#15

@@ +63,5 @@
> +{
> +  if (index >= mPolicies.Length()) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  mPolicies[index]->toString(aStr);

is there any way toString can fail?  If so, we should handle the error (and return an NS_ERROR).

@@ +78,5 @@
> +NS_IMETHODIMP
> +nsCSPContext::RemovePolicy(uint32_t index)
> +{
> +  if (index >= mPolicies.Length()) {
> +    return NS_ERROR_FAILURE;

NS_ERROR_ILLEGAL_VALUE for out of bounds

@@ +88,5 @@
> +NS_IMETHODIMP
> +nsCSPContext::AppendPolicy(const nsAString &policyString,
> +                           nsIURI *selfURI,
> +                           bool reportOnly,
> +                           bool specCompliant)

nit: aPolicyString, aSelfURI, aIsReportOnly, aIsSpecCompliant please.  Also check other parameters throughout the file and use a consistent naming scheme.  (Copy the new arg names to the .h file)

Also, * goes with the type in this file.  Check for that everywhere.

@@ +90,5 @@
> +                           nsIURI *selfURI,
> +                           bool reportOnly,
> +                           bool specCompliant)
> +{
> +  NS_ASSERTION(selfURI, "no self uri");

Assert the requirement, not the error please.  "SelfURI required for AppendPolicy" for example.

@@ +91,5 @@
> +                           bool reportOnly,
> +                           bool specCompliant)
> +{
> +  NS_ASSERTION(selfURI, "no self uri");
> +  nsCSPPolicy *policy = nsCSPParser::parseContentSecurityPolicy(policyString, selfURI, reportOnly, 0);

Can this generate an error?  If so, we should check and handle it here.

@@ +98,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsCSPContext::GetAllowsInlineScript(bool *shouldReportViolations,
> +                                    bool *allowsInlineScript)

outparams should start with o: oShouldReportViolation, oAllowsInlineScript

@@ +158,5 @@
> +
> +NS_IMETHODIMP
> +nsCSPContext::PermitsAncestry(nsIDocShell *docShell, bool *permitsAncestry)
> +{
> +  // TODO: we have to fix this

Mention this will be fixed in bug 994322, and that for now, all ancestors are allowed.

@@ +163,5 @@
> +  *permitsAncestry = true;
> +  return NS_OK;
> +}
> +
> +/* ========== more from nsIContentSecurityPolicy implementation ========== */

Remove "more" and move this to the top of the list of functions from nsIContentSecurityPolicy

::: content/base/src/nsDocument.cpp
@@ +2826,5 @@
> +    if (oldHeaderIsPresent && !newHeaderIsPresent) {
> +      // New parser doesn't support old header!  ABORT CSP INIT!
> +      // (Not a problem if newHeaderIsPresent because the old header will be
> +      // ignored).  This check will get removed when x- header support is
> +      // removed.

Nit: mention bug 949533 is where the x- header support is removed.

@@ +2830,5 @@
> +      // removed.
> +#ifdef PR_LOGGING
> +      PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("%s %s %s",
> +           "This document has an old, x-content-security-policy",
> +           "header and the new parser doesn't support the non-standard",

s/parser/CSP implementation/

::: dom/ipc/preload.js
@@ +70,5 @@
>    Cc["@mozilla.org/thread-manager;1"].getService(Ci["nsIThreadManager"]);
>    Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci["nsIAppStartup"]);
>    Cc["@mozilla.org/uriloader;1"].getService(Ci["nsIURILoader"]);
>    Cc["@mozilla.org/contentsecuritypolicy;1"].createInstance(Ci["nsIContentSecurityPolicy"]);
> +  Cc["@mozilla.org/csp;1"].createInstance(Ci["nsIContentSecurityPolicy"]);

I don't think you need this because it's not a service.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8405638 [details] [diff] [review]
part_2_contentpolicy_tiein.patch

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

I'll need to spend more time with this next week, but it's looking pretty good.  Feel free to make changes based on these comments, or just wait until I've finished and changed the r? flag.

If Garrett "owns" this patch, Chris should be reviewing and not making changes.  Otherwise, please update the user in the patch header to be Chris. :)  Also update the commit message to include bug number.

::: content/base/src/nsCSPContext.cpp
@@ +35,4 @@
>  
>  using namespace mozilla;
>  
> +static const char* CSP_VIOLATION_TOPIC = "csp-on-violate-policy";

This should probably be in nsIContentSecurityPolicy.idl (see review for stubs patch).

@@ +40,4 @@
>  /* ========== nsISupports implementation ========== */
> +NS_IMPL_CLASSINFO(nsCSPContext, nullptr, nsIClassInfo::MAIN_THREAD_ONLY,
> +                  NS_CSPCONTEXT_CID)
> +NS_IMPL_ISUPPORTS2_CI(nsCSPContext, nsIContentSecurityPolicy, nsISerializable)

Please add a comment above these two lines to say these are needed to implement nsIClassInfo on nsCSPContext (so it can be serialized).

@@ +97,5 @@
>  {
> +  // Please note that we are using the selfURI set in RequestContext
> +  // see Bug 991474
> +  NS_ASSERTION(mSelfURI, "Can not AppendPolicy without mSelfURI");
> +  nsCSPPolicy *policy = nsCSPParser::parseContentSecurityPolicy(policyString, mSelfURI, reportOnly, 0);

If we're using selfURI from SetRequestContext, please remove the assertion or turn it into an NS_WARNING that gets spit out when the value passed in is not nullptr (so we stop trying to use the parameter).

@@ +107,5 @@
>  nsCSPContext::GetAllowsInlineScript(bool *shouldReportViolations,
>                                      bool *allowsInlineScript)
>  {
> +  *shouldReportViolations = false;
> +  *allowsInlineScript = true;

Please put a comment here that explains we allow inline scripts unless one of the policies explicitly forbids it (that is what these defaults indicate).  Same with the allowsEval and style checks below.

@@ +116,5 @@
> +      // policy is violated: must report the violation and allow the inline
> +      // script if the policy is report-only.
> +      *shouldReportViolations = true;
> +      if (!mPolicies[i]->getReportOnlyFlag())
> +        *allowsInlineScript = false;

brace the if, please.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8405638 [details] [diff] [review]
part_2_contentpolicy_tiein.patch

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

Looks pretty good.  I'd like to see this one more time, though.

::: content/base/src/nsCSPContext.cpp
@@ +181,5 @@
> +  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
> +    if (!mPolicies[i]->allows(aContentType, NONCE, aNonce)) {
> +      *shouldReportViolation = true;
> +      if (!mPolicies[i]->getReportOnlyFlag())
> +        *allowsNonce = false;

Are the semantics of allowsNonce and allowsHash the same as allowsEval and such?  If so, there *must* be a way to abstract out these loops; all the functions look the same.  Maybe via macro?

@@ +241,5 @@
> +  }
> +
> +  NS_ASSERTION(mSelfURI, "No selfURI in SetRequestContext");
> +
> +  return NS_OK;

I assume the rest of the data gathering (referrer, channel, etc) will be done by future patches?

@@ +273,5 @@
> +  // * Content Type is not whitelisted (CSP Reports, TYPE_DOCUMENT, etc).
> +  // * Fast Path for Apps
> +
> +
> +  // TODO(sid): implement report-only support

Please remove this TODO if report-only support is supported by this patch.

@@ +286,5 @@
> +  // nonce.
> +  nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryInterface(aRequestContext);
> +  bool noncePreload = doc && (aContentType == nsIContentPolicy::TYPE_SCRIPT ||
> +                              aContentType == nsIContentPolicy::TYPE_STYLESHEET);
> +  for (uint32_t p = 0; p < mPolicies.Length(); p++) {

This loop is going through the policies and making sure they *all* permit the load, right?

@@ +289,5 @@
> +                              aContentType == nsIContentPolicy::TYPE_STYLESHEET);
> +  for (uint32_t p = 0; p < mPolicies.Length(); p++) {
> +    if (!mPolicies[p]->permits(aContentType, aContentLocation, violatedDirective)) {
> +
> +      if (!noncePreload) {

Please put a comment before this if block to summarize what it's doing.

@@ +302,5 @@
> +            bool allowsNonce = false;
> +            GetAllowsNonce(nonce, aContentType, &shouldReportViolation, &allowsNonce);
> +            if (allowsNonce) {
> +              break;
> +            }

if GetAllowsNonce returns an nsresult, please check it for errors.  Why do we break when nonce is allowed?  Do we stop checking the other policies or should this be "continue" instead of "break"?

@@ +309,5 @@
> +      }
> +
> +      // the policy does not allow the load, reject the load and report to console
> +      if (!mPolicies[p]->getReportOnlyFlag())
> +        *aDecision = nsIContentPolicy::REJECT_SERVER;

Nit: please brace the if.

@@ +322,5 @@
> +
> +      observerService->NotifyObservers(aContentLocation,
> +                                       CSP_VIOLATION_TOPIC,
> +                                       violatedDirective.get());
> +      // TODO(sid): console error reporting

For the violation reporting TODOs, please reference bug 994322 in the comment.

@@ -199,5 @@
> -                                     uint32_t flags,
> -                                     nsIAsyncVerifyRedirectCallback *callback)
> -{
> -  return NS_ERROR_NOT_IMPLEMENTED;
> -}

Since we're not implementing nsIChannelEventSink, please remove it from the backend_stubs patch too.

@@ +394,5 @@
> +
> +  // NOTE: the document instance that's deserializing this object (via its
> +  // principal) should hook itself into this._principal manually.  If they
> +  // don't, the CSP reports will likely be blocked by nsMixedContentBlocker.
> +  return NS_OK;

Since we're not storing the principal, please remove this comment.

@@ +403,5 @@
>  {
> +  // need to serialize the context: request origin and such. They are
> +  // used when sending reports.  Since _request and _requestOrigin are just
> +  // different representations of the same thing, only save _requestOrigin
> +  // (an nsIURI).

Please update this comment to reference the right class members (not _request and _requestOrigin)

@@ +411,5 @@
> +                                               true);
> +  if (NS_FAILED(rv)) {
> +    fprintf(stderr, "OMG, failed to write mSelfURI");
> +  }
> +  NS_ENSURE_SUCCESS(rv, rv);

Please remove this if block, it's unnecessary with the NS_ENSURE_SUCCESS.

@@ +416,5 @@
> +
> +  // we can't serialize a reference to the principal that triggered this
> +  // instance to serialize, so when this is deserialized by the principal the
> +  // caller must hook it up manually by calling setRequestContext on this
> +  // instance with the appropriate nsIChannel.

If we're not storing the principal anyway, please remove this comment or change it to say "some things aren't stored during serialization, they must be restored by calling SetRequestContext on this instance after deserialization".

::: content/base/src/nsCSPUtils.cpp
@@ +655,5 @@
>    }
>  
> +  // if [frame-ancestors] is not listed explicitly then default to true
> +  // without consulting [default-src]
> +  // TODO: currently [frame-ancestors] is mapped to TPE_DOCUMENT (needs to be fixed)

Typo: TPE_DOCUMENT should be TYPE_DOCUMENT.  Also, do we know how to fix this or should we remove the TODO and instead put in:

// XXX: [frame-ancestors] is mapped generically to TYPE_DOCUMENT, 
// there may be a less-hacky way to do this, but for now this is 
// what nsIContentPolicy types provide.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8404140 [details] [diff] [review]
part_6_parser_tests.patch

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

This is really close.  I'd like to see it one more time.

Can you factor even more out of each test?  Each test finishes with a loop that checks the results; can you either make a macro generate that part (so the code only appears once) or a function?

Does it make any sense to move all the tests from test_csputils.js (xpcshell test) into this compiled code test?  There are a bunch in there.

::: content/base/test/TestCSPParser.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MOZILLA_INTERNAL_API
> +// some of the includes make use of internal string types

please add more to this comment: so we need to forward-declare some stuff.

@@ +57,5 @@
> +  while (start != end) {
> +    fprintf(stderr, "%c", *start++);
> +  }
> +  fprintf(stderr, "\n\n\n");
> +}

Do the other compiled-code tests use fprintf, or is there another logging mechanism we should use?

@@ +59,5 @@
> +  }
> +  fprintf(stderr, "\n\n\n");
> +}
> +
> +const int CHECK_POLICY_AT_INDEX_0 = 0;

This is only used in one location, please just hard-code it and put a comment there to say why you're passing 0.

@@ +74,5 @@
> +  // create a CSP object
> +  nsCOMPtr<nsIContentSecurityPolicy> csp =
> +    do_CreateInstance(NS_CSPCONTEXT_CONTRACTID, &rv);
> +  if (NS_FAILED(rv))
> +    return rv;

NS_ENSURE_SUCCESS(rv, rv) works fine.  Or brace your if.

@@ +78,5 @@
> +    return rv;
> +
> +  csp->SetRequestContext(selfURI, nullptr, nullptr, nullptr);
> +  if (NS_FAILED(rv))
> +    return rv;

Same here.  NS_ENSURE_SUCCESS(rv, rv);

@@ +99,5 @@
> +    if (NS_FAILED(rv))
> +      return rv;
> +
> +    if (actualPolicyCount != expectedPolicyCount)
> +      return NS_ERROR_FAILURE;

NS_ENSURE_FALSE(actualPolicyCount == expectedPolicyCount, NS_ERROR_FAILURE);

@@ +147,5 @@
> +    { "script-src 'sha256-siVR8vAcqP06h2ppeNwqgjr0yZ6yned4X2VF84j4GmI='",
> +      "script-src 'sha256-siVR8vAcqP06h2ppeNwqgjr0yZ6yned4X2VF84j4GmI=';" }
> +  };
> +
> +  uint32_t size = sizeof(policies) / sizeof(PolicyTest);

"size" is a weird name for this variable.  Maybe "policyCount" or something?

Comment 18

[Garrett Robinson [:grobinson]]

Comment on attachment 8404134 [details] [diff] [review]
part_0_parser_utils.patch

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

General nit: trailing whitespace.

r=me with fixes, and make sure you add code to handle bug 885433! (is this not being caught by test_CSP_bug885433?)

::: content/base/src/nsCSPParser.cpp
@@ +98,5 @@
> +#else
> +#define DUMP_POLICY(aPolicyString, aSelfURI, aReportOnly)
> +#define DUMP_FUNC(aFuncName)
> +#define DUMP_ERROR(aMsg, aDirective, aToken, aValue, aChar)
> +#endif

+1, this is exactly what PR logging is meant for.

@@ +146,5 @@
> +  return false;
> +}
> +
> +bool isQuoteLessKeyword(const nsAString& aKey) {
> +  nsString lowerKey = PromiseFlatString(aKey);

Is there any reason to use PromiseFlatString (described as a "last resort") instead of just copying the string?

@@ +150,5 @@
> +  nsString lowerKey = PromiseFlatString(aKey);
> +  ToLowerCase(lowerKey);
> +
> +  uint32_t length = sizeof(KEYWORDS) / sizeof(KEYWORDS[0]);
> +  nsString keyword;

You're encouraged to use nsAutoString for local variables to save allocations where possible: https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#Local_variables.

@@ +200,5 @@
> +      nsAutoCString prePath;
> +      aSelfURI->GetPrePath(prePath);
> +
> +      const char16_t* params[] = { NS_ConvertUTF8toUTF16(prePath).get() };
> +      parser->logWarningErrorToConsole(nsIScriptError::warningFlag, "reportURInotInReportOnlyHeader",

If you pass aInnerWindowID to this logging function, the message will appear in the (global) Browser Console and will also be correctly routed to the page's corresponding Web Console.

@@ +306,5 @@
> +  }
> +
> +  // create a new CSP-Directive and add name
> +  const uint32_t NAME_INDEX = 0;
> +  nsString dirName = aTokens[NAME_INDEX];

NAME_INDEX isn't used again, so it's unnecessary, just use 0-index (maybe with a comment, but it's clear imo).

@@ +341,5 @@
> +{
> +  DUMP_FUNC("directiveName");
> +  if (!isDirective(aName)) {
> +    DUMP_ERROR(INVALID_KEYWORD, mCurDirName, mCurToken, mCurValue, *mCurChar);
> +    const char16_t* params[] = { PromiseFlatString(aName).get() };

I think the NS_Convert macros are generally preferred over PromiseFlatString. But you don't need to convert here, so why can't you just do aName.get()?

@@ +371,5 @@
> +    nsString value = aTokens[i];
> +
> +    // special case handling for none
> +    if (value.LowerCaseEqualsASCII(NONE)) {
> +      isNone = true;

We discussed this a while back. 'none' is a valid source-list (*not* source-expression), so 'none' with other tokens is invalid, 'none' should be ignored and the other tokens enforced. This appears to be correctly implemented at the end of this function.

@@ +398,5 @@
> +  // ignore 'none' if any other src is available
> +  if (isNone && outSrcs.Length() == 0) {
> +    nsString none;
> +    none.AssignASCII(NONE);
> +    nsCSPKeywordSrc *keyword = new nsCSPKeywordSrc(none);

I don't think this is correct, although it works. 'none' is not a keyword, and it confuses the mapping between ABNF and the code to make it one. imo it would make sense to have an isNone flag on the *directive* object, or maybe on the source list.

@@ +459,5 @@
> +}
> +
> +// Helper function for nonce and hash
> +bool
> +startsWith(const nsAString& aString, const nsAString& aPrefix) {

Didn't know about that file! Thanks! (I am excited because I wrote this, and was annoyed by the seeming lack of such utility functions).

@@ +493,5 @@
> +nsCSPHashSource*
> +nsCSPParser::hashSource(const nsAString& aExpr)
> +{
> +  DUMP_FUNC("hashSource");
> +  if (!startsWith(aExpr, NS_LITERAL_STRING("'"))) {

Just ', because it could be any one of the supported hash functions (e.g. 'sha256-foo', 'sha512-foo'). This is handled in the loop over kHashSourceValidFns below.

@@ +599,5 @@
> +{
> +  DUMP_FUNC("host");
> +
> +  if (accept(WILDCARD)) {
> +    // might solely the wildcard

Nit: I think you a word :)

@@ +651,5 @@
> +    }
> +    nsCSPHostSrc *cspHost = new nsCSPHostSrc(aScheme);
> +
> +    if (isQuoteLessKeyword(mCurValue)) {
> +      nsXPIDLString errorMsg;

This isn't used?

::: content/base/src/nsCSPUtils.cpp
@@ +37,5 @@
> +  // want debugging printed to the terminal, uncomment:
> +  //printf("%s\n", aMsg);
> +}
> +
> +void CSP_GetLocalizedStr(const char16_t* aName,

Nit: return type on its own line

@@ +100,5 @@
> +                            aSourceLine, aLineNumber,
> +                            aColumnNumber, aFlags,
> +                            catStr, aInnerWindowID);
> +  }
> +  else {

Nit: remove newline before else, so you have "} else {"

@@ +150,5 @@
> +
> +bool
> +nsCSPSchemeSrc::allows(enum AllowsType aType, const nsAString& aStr) const
> +{
> +  return false;

It doesn't implement that, only keyword, nonce, and hash are used with allows. This is a consequence of having everything descend from a CSPBaseSrc class - all functions must be implemented.

@@ +571,5 @@
> +    return nsIContentPolicy::TYPE_OBJECT;
> +  }
> +
> +  if (aName.EqualsASCII(FRAME_SRC)) {
> +    return nsIContentPolicy::TYPE_SUBDOCUMENT;

This type of code is better written with a switch or by indexing a map, but that might be more trouble than it's worth in C++-land.

@@ +702,5 @@
> +  // Bug 885433
> +  // a) inline scripts (also unsafe eval) should only be blocked
> +  //    if there is a [script-src] or [default-src]
> +  // b) inline styles should only be blocked
> +  //    if there is a [style-src] or [default-src]

This should be implemented before landing so we don't have a regression.

::: content/base/src/nsCSPUtils.h
@@ +47,5 @@
> +//   if (!gCSPLog)
> +//     gCSPLog = PR_NewLogModule("CSP");
> +//   return gCSPLog;
> +// }
> +// #define CSP_PR_LOG(type, msg) PR_LOG(GetCSPPRLog(), type, msg)

This was working when I originally wrote it, wonder what happened.

Comment 19

[Garrett Robinson [:grobinson]]

Comment on attachment 8404135 [details] [diff] [review]
part_1_backend_stubs.patch

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

Agree with Sid's comments, otherwise lgtm.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_0_parser_utils_v2.patch

Hey Sid,
thanks for the first round of feedback. I guess I incorporated all of your suggestions:
* Eliminated defines and use static const char*
* Using NS_ENSURE_SUCCESS wherever possible
* Using the URI-Parser for policy-uri
* Simplyfied wherever possible and flipped the parser around, most detailed function on top, all the way down to parseContentSecurityPolicy; that really makes it easier to read.
* Documented the parser very detailed.
* Fixed all the nits.
* and so much more.

This patch is ready for review round two.

Comment 21

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8408579 [details] [diff] [review]
part_0_parser_utils_v2.patch

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

Overall, this is looking really good.  I'd like to see it one more time, though.  Please address the comments below and put a comment on this bug with answers for any questions posed.

::: content/base/src/nsCSPParser.cpp
@@ +57,5 @@
> +bool isCharacterToken(char16_t aSymbol)
> +{
> +#ifdef PR_LOGGING
> +  PR_LOG(gCspParserPRLog, PR_LOG_DEBUG,
> +        ("nsCSPParser::isCharacterToken: %c", aSymbol));

This function isn't a member of nsCSPParser, don't print it as such unless it is.

@@ +77,5 @@
> +
> +  if (aSymbol >= '0' && aSymbol <= '9') {
> +    return true;
> +  }
> +  return false;

Please please don't do this (I mentioned this in the previous, monolithic insane review comment):
  if (x) { return true; } return false;
should be
  return x;

Fix this everywhere in this file.

@@ +105,5 @@
> +  if (*mCurChar == aSymbol) {
> +    advance();
> +    return true;
> +  }
> +  return false;

I see advance() used a lot with return true; where we could optimize the code a bit.  If advance returned a boolean instead of void, we could rewrite:
 if (foo) {
   advance();
   return true;
 }
 return false;
into:
 return foo && advance();

What do you think? (I think this got lost from the previous review.)

@@ +192,5 @@
> +#endif
> +
> +  return (accept(isCharacterToken) ||
> +          accept(isNumberToken) ||
> +          accept(DASH));

I like this use of function pointers!  Slick.

@@ +228,5 @@
> +#endif
> +
> +  // If the current host does not enforce a port, we can return right away.
> +  if (!accept(COLON)) {
> +    return true;

What if port() is called before host()?  Do we need to ensure somehow that that doesn't happen? 
For example: "foo:23" is a valid host/port, but calling port("foo:23") will return true but not parse a port...
Please ensure that this doesn't cause problems (require host called before port) or put a comment here explaining why it's unnecessary.

@@ +325,5 @@
> +
> +// host = "*" / [ "*." ] 1*host-char *( "." 1*host-char )
> +nsCSPHostSrc*
> +nsCSPParser::host(const nsAString& aScheme)
> +{

This function is much easier to read now, thanks!

@@ +334,5 @@
> +  // Check if the token starts with "*"
> +  if (accept(WILDCARD)) {
> +    // Might solely be the wildcard
> +    if (peek(*mEndChar)) {
> +      nsCSPHostSrc *cspHost = new nsCSPHostSrc(aScheme);

This feels awkward to pass the scheme into the nsCSPHostSrc constructor, but not the host.  Why are you not passing both?

@@ +394,5 @@
> +    return CSP_CreateHostSrcFromURI(mSelfURI);
> +  }
> +
> +  if (aKeyword.LowerCaseEqualsASCII(CSP_EnumToKeyword(CSP_UNSAFE_INLINE)) ||
> +      aKeyword.LowerCaseEqualsASCII(CSP_EnumToKeyword(CSP_UNSAFE_EVAL))) {

Can we make a macro/function to compare a string to keyword?  CSP_IS_KEYWORD(aKeyword, SELF), for example...

@@ +439,5 @@
> +
> +  if (!hostChar()) {
> +    return nullptr;
> +  }
> +  while (hostChar()) { /* consume */ }

Are all hostchars valid for schemes?  I don't know the answer off the top of my head, but I suspect not.  The RFC is here: http://www.ietf.org/rfc/rfc3986.txt , see section 3.1

@@ +535,5 @@
> +
> +  // Check if it is a hash-source
> +  if (nsCSPHashSource* cspHash = hashSource(aExpression)) {
> +    return cspHash;
> +  }

Is there ever a chance that our code will accidentally parse one source type as a keyword/nonce/hash source accidentally?  I'm thinking "no", but if I'm wrong, we need stronger error checking inside the parsers for keyword, nonce and source.

@@ +562,5 @@
> +
> +    // If aExpression provides not only a scheme, but also a host, we have to check
> +    // if two slashes follow the scheme.
> +    if (!accept(SLASH) || !accept(SLASH)) {
> +      const char16_t* params[] = { PromiseFlatString(aExpression).get() };

Is there a reason PromiseFlatString is used here instead of just aExpression.get()?

@@ +615,5 @@
> +
> +    // Special case handling for none
> +    if (value.LowerCaseEqualsASCII(CSP_EnumToKeyword(CSP_NONE))) {
> +      isNone = true;
> +      continue;

What happens if the list contains 'none' and other tokens?  What does the spec say to do (do that here and explain it in a comment).

@@ +648,5 @@
> +    nsString token = aTokens[i];
> +
> +    // Special case handling for relative URIs
> +    if (token.First() == SLASH) {
> +      // Get the spec from selfURI

Why do you need special casing here?  Can't you just pass the relative URL to NS_NewURI?  (Should work as long as you pass in a base URI, a.k.a. self).  This is why I really wanted to use the already-built URI parser...

Example: http://mxr.mozilla.org/mozilla-central/source/netwerk/test/TestPageLoad.cpp#277

::: content/base/src/nsCSPParser.h
@@ +32,5 @@
> + * The first element of each array has to be a valid directive-name, otherwise we can
> + * ignore the remaining elements of the array. Also, if the
> + * directive already exists in the current policy, we can ignore
> + * the remaining elements of that array. (http://www.w3.org/TR/CSP/#parsing)
> + */

Could you also include a short example "how to use the parser"?  Make the entry points clear, show an example of how to get a CSP policy out of a string.

@@ +80,5 @@
> +    bool              hostChar();
> +    // port = ":" ( 1*DIGIT / "*" )
> +    bool              port(nsCSPHostSrc* aCspHost);
> +    // path  = <path production from RFC 3986, section 3.3>
> +    bool              path(nsCSPHostSrc* aCspHost);

Now that you've kindly lined up all the function names, it appears hard to read.  What would you prefer: stand-alone ABNF, (with the functions lined up nicely) or extra spaces between a function declaration and the next comment?

@@ +113,5 @@
> +    // Helper variables/members which are used/reset during parsing
> +    // e.g. parsing: "http://www.example.com"
> +    //                ^                    ^
> +    //                mCurChar             mEndChar
> +    // mCurValue = ""

Excellent.

::: content/base/src/nsCSPUtils.cpp
@@ +67,5 @@
> +  }
> +
> +  // Prepending CSP to the outgoing console message
> +  nsString cspMsg = NS_ConvertUTF8toUTF16("Content Security Policy: ");
> +  cspMsg.Append(aMessage);

Can you just do cspMsg.Append(NS_LITERAL_STRING("Content Security Policy: "))?

@@ +637,5 @@
> +  // We haven't found a matching directive name, therefore we check the default-src
> +  for (uint32_t i = 0; i < mDirectives.Length(); i++) {
> +    if (mDirectives[i]->isDefaultDirective()) {
> +      if (!mDirectives[i]->permits(aUri)) {
> +        mDirectives[i]->getDirectiveName(outViolatedDirective);

Instead of making a second pass, can you pick up the default directive during the first loop and only use it if the loop completes without finding the specific directive?

@@ +672,5 @@
> +  // Use the default directive
> +  for (uint32_t i = 0; i < mDirectives.Length(); i++) {
> +    if (mDirectives[i]->isDefaultDirective()) {
> +      if (mDirectives[i]->allows(aKeyword, aHashOrNonce)) {
> +        return true;

Same as above, can you just do one loop and pick up the defaultDirective on the way through the first time?

::: content/base/src/nsCSPUtils.h
@@ +34,5 @@
> +
> +/* =============== Definitions ================== */
> +
> +// Please add a string version for every enum
> +// to CSPStrDirectives underneath.

Does the order matter?  I assume it does, and so we should make that explicit in this comment.

@@ +64,5 @@
> +  "connect-src", // CSP_CONNECT_SRC
> +  "report-uri",  // CSP_REPORT_URI
> +};
> +
> +inline const char* CSP_EnumToDirective(enum CSPDirective aDir)

Do these have to be inline functions or can they be macros?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_0_parser_utils_v3.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #21)
> Comment on attachment 8408579 [details] [diff] [review]
> part_0_parser_utils_v2.patch
> 
> Review of attachment 8408579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, this is looking really good.  I'd like to see it one more time,
> though.  Please address the comments below and put a comment on this bug
> with answers for any questions posed.
> 
> ::: content/base/src/nsCSPParser.cpp
> @@ +57,5 @@
> > +bool isCharacterToken(char16_t aSymbol)
> > +{
> > +#ifdef PR_LOGGING
> > +  PR_LOG(gCspParserPRLog, PR_LOG_DEBUG,
> > +        ("nsCSPParser::isCharacterToken: %c", aSymbol));
> 
> This function isn't a member of nsCSPParser, don't print it as such unless
> it is.

Fixed, also for isNumberToken.

> @@ +77,5 @@
> > +
> > +  if (aSymbol >= '0' && aSymbol <= '9') {
> > +    return true;
> > +  }
> > +  return false;
> 
> Please please don't do this (I mentioned this in the previous, monolithic
> insane review comment):
>   if (x) { return true; } return false;
> should be
>   return x;
> 
> Fix this everywhere in this file.

Totally; fixed it everywhere.
 
> @@ +105,5 @@
> > +  if (*mCurChar == aSymbol) {
> > +    advance();
> > +    return true;
> > +  }
> > +  return false;
> 
> I see advance() used a lot with return true; where we could optimize the
> code a bit.  If advance returned a boolean instead of void, we could rewrite:
>  if (foo) {
>    advance();
>    return true;
>  }
>  return false;
> into:
>  return foo && advance();
> 
> What do you think? (I think this got lost from the previous review.)

Well, it didn't got lost actually. But I haven't commented on it.
In my opinion it doesn't make sense semantically to let advance() return a boolean, because it can never fail. Modern compilers are smart enough and produce the same code for both of our versions. I did the change anyway. Looking at the statement itself:
> return foo && advance();
looks good though.


> @@ +228,5 @@
> > +#endif
> > +
> > +  // If the current host does not enforce a port, we can return right away.
> > +  if (!accept(COLON)) {
> > +    return true;
> 
> What if port() is called before host()?  Do we need to ensure somehow that
> that doesn't happen? 
> For example: "foo:23" is a valid host/port, but calling port("foo:23") will
> return true but not parse a port...
> Please ensure that this doesn't cause problems (require host called before
> port) or put a comment here explaining why it's unnecessary.

The parser has only one entry point (one public interface function, which is parseContentSecurityPolicy).
Since the parser is still (kind of) a Finite State Machine, port can not be called before host. I put a comment to make this explicit in nsCSPParser.h

> @@ +334,5 @@
> > +  // Check if the token starts with "*"
> > +  if (accept(WILDCARD)) {
> > +    // Might solely be the wildcard
> > +    if (peek(*mEndChar)) {
> > +      nsCSPHostSrc *cspHost = new nsCSPHostSrc(aScheme);
> 
> This feels awkward to pass the scheme into the nsCSPHostSrc constructor, but
> not the host.  Why are you not passing both?

Or nothing at all :-). I restructured parts of the parser again to make use of the internal state of the object which allows us to avoid passing unnecessary arguments around. Thanks for the hint, sometimes coding makes you not see the obvious. Take a look at the nsCSPParser.h file, looks way better/cleaner now.
 
> @@ +394,5 @@
> > +    return CSP_CreateHostSrcFromURI(mSelfURI);
> > +  }
> > +
> > +  if (aKeyword.LowerCaseEqualsASCII(CSP_EnumToKeyword(CSP_UNSAFE_INLINE)) ||
> > +      aKeyword.LowerCaseEqualsASCII(CSP_EnumToKeyword(CSP_UNSAFE_EVAL))) {
> 
> Can we make a macro/function to compare a string to keyword? 
> CSP_IS_KEYWORD(aKeyword, SELF), for example...

Sure can, and of course did.

> @@ +439,5 @@
> > +
> > +  if (!hostChar()) {
> > +    return nullptr;
> > +  }
> > +  while (hostChar()) { /* consume */ }
> 
> Are all hostchars valid for schemes?  I don't know the answer off the top of
> my head, but I suspect not.  The RFC is here:
> http://www.ietf.org/rfc/rfc3986.txt , see section 3.1

Oh oh, that is an awesome catch. Thanks for reporting this. hostChar() is not at all correct here.
I added a new member function, schemeChar() to handle this correctly. This actually helped me to find some nifty bugs in the parser.
 
> @@ +535,5 @@
> > +
> > +  // Check if it is a hash-source
> > +  if (nsCSPHashSource* cspHash = hashSource(aExpression)) {
> > +    return cspHash;
> > +  }
> 
> Is there ever a chance that our code will accidentally parse one source type
> as a keyword/nonce/hash source accidentally?  I'm thinking "no", but if I'm
> wrong, we need stronger error checking inside the parsers for keyword, nonce
> and source.

I really don't think there is any possibility that the current meachnism would accidentaly classify a keyword/hash/nonce.
 
> @@ +562,5 @@
> > +
> > +    // If aExpression provides not only a scheme, but also a host, we have to check
> > +    // if two slashes follow the scheme.
> > +    if (!accept(SLASH) || !accept(SLASH)) {
> > +      const char16_t* params[] = { PromiseFlatString(aExpression).get() };
> 
> Is there a reason PromiseFlatString is used here instead of just
> aExpression.get()?

The reason we had to use PromiseFlatString() was because previously function arguments like aExpression in this case were declard as |nsAString& aExpression|. Since nsAString do not provide the .get() function we had to convert the string using nsPromiseFlatString(). The new structure of the parser took care of the problem. Since we do not pass strings as arguments internally anymore, we do not have the problem. Awesome :-)

> 
> @@ +615,5 @@
> > +
> > +    // Special case handling for none
> > +    if (value.LowerCaseEqualsASCII(CSP_EnumToKeyword(CSP_NONE))) {
> > +      isNone = true;
> > +      continue;
> 
> What happens if the list contains 'none' and other tokens?  What does the
> spec say to do (do that here and explain it in a comment).

Oh, the comment is a little further down in that method. I pushed it up to be on top of the if-check.
 
> @@ +648,5 @@
> > +    nsString token = aTokens[i];
> > +
> > +    // Special case handling for relative URIs
> > +    if (token.First() == SLASH) {
> > +      // Get the spec from selfURI
> 
> Why do you need special casing here?  Can't you just pass the relative URL
> to NS_NewURI?  (Should work as long as you pass in a base URI, a.k.a. self).
> This is why I really wanted to use the already-built URI parser...
> 
> Example:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/test/TestPageLoad.
> cpp#277

Thanks for the link, didn't know it's that easy to provide a baseURI. Simplyfies things drastically.
 
> ::: content/base/src/nsCSPParser.h
> @@ +32,5 @@
> > + * The first element of each array has to be a valid directive-name, otherwise we can
> > + * ignore the remaining elements of the array. Also, if the
> > + * directive already exists in the current policy, we can ignore
> > + * the remaining elements of that array. (http://www.w3.org/TR/CSP/#parsing)
> > + */
> 
> Could you also include a short example "how to use the parser"?  Make the
> entry points clear, show an example of how to get a CSP policy out of a
> string.

Extended/Added comments; should be clear now.
 
> @@ +80,5 @@
> > +    bool              hostChar();
> > +    // port = ":" ( 1*DIGIT / "*" )
> > +    bool              port(nsCSPHostSrc* aCspHost);
> > +    // path  = <path production from RFC 3986, section 3.3>
> > +    bool              path(nsCSPHostSrc* aCspHost);
> 
> Now that you've kindly lined up all the function names, it appears hard to
> read.  What would you prefer: stand-alone ABNF, (with the functions lined up
> nicely) or extra spaces between a function declaration and the next comment?

I think we should have the ABNF only in the *.cpp file. I removed it from the *.h file. Makes things easier to read in my opinion. In addition, I rather have it above the current function/method I am looking at in the cpp file anyway. Are you OK with that? If not, we can change it again, no problem.
 
> @@ +113,5 @@
> > +    // Helper variables/members which are used/reset during parsing
> > +    // e.g. parsing: "http://www.example.com"
> > +    //                ^                    ^
> > +    //                mCurChar             mEndChar
> > +    // mCurValue = ""
> 
> Excellent.

Added even more comments to explain in detail.
 
> ::: content/base/src/nsCSPUtils.cpp
> @@ +67,5 @@
> > +  }
> > +
> > +  // Prepending CSP to the outgoing console message
> > +  nsString cspMsg = NS_ConvertUTF8toUTF16("Content Security Policy: ");
> > +  cspMsg.Append(aMessage);
> 
> Can you just do cspMsg.Append(NS_LITERAL_STRING("Content Security Policy:
> "))?

Sure can, and did :-)
 
> @@ +637,5 @@
> > +  // We haven't found a matching directive name, therefore we check the default-src
> > +  for (uint32_t i = 0; i < mDirectives.Length(); i++) {
> > +    if (mDirectives[i]->isDefaultDirective()) {
> > +      if (!mDirectives[i]->permits(aUri)) {
> > +        mDirectives[i]->getDirectiveName(outViolatedDirective);
> 
> Instead of making a second pass, can you pick up the default directive
> during the first loop and only use it if the loop completes without finding
> the specific directive?

Most certainly can. Incorporated that for ::allows and ::permits.

> > +// Please add a string version for every enum
> > +// to CSPStrDirectives underneath.
> 
> Does the order matter?  I assume it does, and so we should make that
> explicit in this comment.

Indeed, order matters. Extended the comment so no one can mess with it.
 
> @@ +64,5 @@
> > +  "connect-src", // CSP_CONNECT_SRC
> > +  "report-uri",  // CSP_REPORT_URI
> > +};
> > +
> > +inline const char* CSP_EnumToDirective(enum CSPDirective aDir)
> 
> Do these have to be inline functions or can they be macros?

I leave them as inlines for now, what would be the advantage of having a macro?
If you have strong arguments, I will change that of course.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_1_backend_stubs_v2.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #14)
> Comment on attachment 8404135 [details] [diff] [review]
> part_1_backend_stubs.patch
> 
> Review of attachment 8404135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the changes mentioned here and grobinson's feedback.
> 
> Please update the commit message to include bug number and include some
> language about this being the new C++ CSP implementation.
> 
> ::: content/base/src/nsCSPContext.cpp
> @@ +26,5 @@
> > +#include "nsIAsyncVerifyRedirectCallback.h"
> > +#include "nsAsyncRedirectVerifyHelper.h"
> > +#include "mozilla/Preferences.h"
> > +#include "nsIScriptError.h"
> > +#include "nsContentUtils.h"
> 
> Please alphabetize the includes... so long as it doesn't break stuff.

Most certainly.
 
> @@ +36,5 @@
> > +                   nsIContentSecurityPolicy,
> > +                   nsIChannelEventSink,
> > +                   nsISerializable)
> > +
> > +static const char* CSP_VIOLATION_TOPIC = "csp-on-violate-policy";
> 
> Please either make this a #define in nsCSPContext.h or (better) make it a
> constant in nsIContentSecurityPolicy!
> For example:
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/
> nsIGlobalHistory2.idl#15

I have put it in the *.idl.
 
> @@ +63,5 @@
> > +{
> > +  if (index >= mPolicies.Length()) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +  mPolicies[index]->toString(aStr);
> 
> is there any way toString can fail?  If so, we should handle the error (and
> return an NS_ERROR).

toString itself can't fail, unless an nsString would fail to alloc memory. Since there are no error codes for nsString I guess it doesn't make sense that toString() returns an error. I will gladly change that though if you tell me what kind of errors we could check for.
 
> @@ +78,5 @@
> > +NS_IMETHODIMP
> > +nsCSPContext::RemovePolicy(uint32_t index)
> > +{
> > +  if (index >= mPolicies.Length()) {
> > +    return NS_ERROR_FAILURE;
> 
> NS_ERROR_ILLEGAL_VALUE for out of bounds

Changed that!
 
> @@ +88,5 @@
> > +NS_IMETHODIMP
> > +nsCSPContext::AppendPolicy(const nsAString &policyString,
> > +                           nsIURI *selfURI,
> > +                           bool reportOnly,
> > +                           bool specCompliant)
> 
> nit: aPolicyString, aSelfURI, aIsReportOnly, aIsSpecCompliant please.  Also
> check other parameters throughout the file and use a consistent naming
> scheme.  (Copy the new arg names to the .h file)
> 
> Also, * goes with the type in this file.  Check for that everywhere.

Changed in/out-params in all the functions to be consistent.
 
> @@ +90,5 @@
> > +                           nsIURI *selfURI,
> > +                           bool reportOnly,
> > +                           bool specCompliant)
> > +{
> > +  NS_ASSERTION(selfURI, "no self uri");
> 
> Assert the requirement, not the error please.  "SelfURI required for
> AppendPolicy" for example.

Changed that of course - makes sense!

 
> @@ +91,5 @@
> > +                           bool reportOnly,
> > +                           bool specCompliant)
> > +{
> > +  NS_ASSERTION(selfURI, "no self uri");
> > +  nsCSPPolicy *policy = nsCSPParser::parseContentSecurityPolicy(policyString, selfURI, reportOnly, 0);
> 
> Can this generate an error?  If so, we should check and handle it here.
> 
> @@ +98,5 @@
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsCSPContext::GetAllowsInlineScript(bool *shouldReportViolations,
> > +                                    bool *allowsInlineScript)
> 
> outparams should start with o: oShouldReportViolation, oAllowsInlineScript

Since we are using out as a prefix in the parser, I am also prefix outgoings params with out here.
 
> @@ +158,5 @@
> > +
> > +NS_IMETHODIMP
> > +nsCSPContext::PermitsAncestry(nsIDocShell *docShell, bool *permitsAncestry)
> > +{
> > +  // TODO: we have to fix this
> 
> Mention this will be fixed in bug 994322, and that for now, all ancestors
> are allowed.

Did that, but it's bug 994320 :-)
 
> @@ +163,5 @@
> > +  *permitsAncestry = true;
> > +  return NS_OK;
> > +}
> > +
> > +/* ========== more from nsIContentSecurityPolicy implementation ========== */
> 
> Remove "more" and move this to the top of the list of functions from
> nsIContentSecurityPolicy

Easy fix.
 
> ::: content/base/src/nsDocument.cpp
> @@ +2826,5 @@
> > +    if (oldHeaderIsPresent && !newHeaderIsPresent) {
> > +      // New parser doesn't support old header!  ABORT CSP INIT!
> > +      // (Not a problem if newHeaderIsPresent because the old header will be
> > +      // ignored).  This check will get removed when x- header support is
> > +      // removed.
> 
> Nit: mention bug 949533 is where the x- header support is removed.

Yep, did that.
 
> @@ +2830,5 @@
> > +      // removed.
> > +#ifdef PR_LOGGING
> > +      PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("%s %s %s",
> > +           "This document has an old, x-content-security-policy",
> > +           "header and the new parser doesn't support the non-standard",
> 
> s/parser/CSP implementation/
> 
> ::: dom/ipc/preload.js
> @@ +70,5 @@
> >    Cc["@mozilla.org/thread-manager;1"].getService(Ci["nsIThreadManager"]);
> >    Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci["nsIAppStartup"]);
> >    Cc["@mozilla.org/uriloader;1"].getService(Ci["nsIURILoader"]);
> >    Cc["@mozilla.org/contentsecuritypolicy;1"].createInstance(Ci["nsIContentSecurityPolicy"]);
> > +  Cc["@mozilla.org/csp;1"].createInstance(Ci["nsIContentSecurityPolicy"]);
> 
> I don't think you need this because it's not a service.

Fixed that as well.

Comment 24

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8409829 [details] [diff] [review]
part_0_parser_utils_v3.patch

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

Awesome documentation in nsCSPParser.h!

r=me with these tweaks.  Sending over to r?jst for someone who's not nose-deep in CSP things.

::: content/base/src/nsCSPParser.cpp
@@ +208,5 @@
> +  if (accept(DOT)) {
> +    if (!accept(isCharacterToken)) {
> +      return false;
> +    }
> +  }

You can simplify this as:
 if (accept(DOT) && !accept(isCharacterToken)) {
   return false;
 }

@@ +463,5 @@
> +  if (!fileAndArguments()) {
> +    return cspHost;
> +  }
> +  // Set the parsed file (and possible arguments) to nsCSPHost.
> +  cspHost->setFileAndArguments(mCurValue);

Please turn this if statement around.

 if (fileAndArguments()) {
   cspHost->setFileAndArguments(mCurValue);
 }
 return cspHost;

@@ +687,5 @@
> +
> +  if (isNone && outSrcs.Length() == 0) {
> +    nsCSPKeywordSrc *keyword = new nsCSPKeywordSrc(CSP_NONE);
> +    outSrcs.AppendElement(keyword);
> +  }

We should probably report a warning here when the "none" keyword is used alongside other sources.  (The logic is right, just need some console logging for web devs.)

@@ +713,5 @@
> +                     mSelfURI);
> +    } else {
> +      // Absolute URI, no selfURI needed as base
> +      rv = NS_NewURI(getter_AddRefs(uri), NS_ConvertUTF16toUTF8(mCurToken).get());
> +    }

I think you can get by with just one call to NS_NewURI (and no if statement)... the baseURI is not used unless it's needed.
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#513

::: content/base/src/nsCSPParser.h
@@ +38,5 @@
> +class nsCSPParser {
> +
> +  public:
> +    /**
> +     * The CSP parser only has one public accessible function, which is parseContentSecurityPolicy.

s/public/publicly/

@@ +75,5 @@
> +    bool              hostChar();
> +    bool              schemeChar();
> +    bool              port();
> +    bool              path(nsCSPHostSrc* aCspHost);
> +    bool              fileAndArguments();

To be clear, these bool-returning functions are not predicates, but state-modifying subroutines, right?  Would it make sense to prefix each with "parse", or is that too wordy?

::: content/base/src/nsCSPUtils.cpp
@@ +650,5 @@
> +      }
> +      return true;
> +    }
> +
> +    // If the loop runs trough, we haven't found a matching directive.

Typo: s/trough/through/
And this comment may belong below where the default-src permits check is actually used.

@@ +658,5 @@
> +      defaultAllows = mDirectives[i]->permits(aUri);
> +      if (!defaultAllows) {
> +        mDirectives[i]->toString(outViolatedDirective);
> +      }
> +    }

What if you just store a pointer to the default directive (instead of querying it right away) and then use it below in the "use default-src if it is defined" section?

So this block would be:
 nsCSPPolicy* defaultDir = nullptr;
 if (mDirectives[i]->isDefaultDirective()) {
   defaultDir = mDirectives[i];
 }

And below:

 if (defaultDir) {
   return defaultDir->permits(aURI);
 }

Just a thought, not critical.  May save some work required for specific policies.  (Same for allows, below).

Comment 25

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8409830 [details] [diff] [review]
part_1_backend_stubs_v2.patch

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

There was probably no need to flag me again because I gave you r+ in the last review of this patch, but I looked anyway.  Just a few minors.  r=me

::: content/base/src/nsCSPContext.cpp
@@ +120,5 @@
> +  NS_ASSERTION(aSelfURI, "aSelfURI required for AppendPolicy");
> +  nsCSPPolicy* policy = nsCSPParser::parseContentSecurityPolicy(aPolicyString, aSelfURI, aReportOnly, 0);
> +  if (policy) {
> +    mPolicies.AppendElement(policy);
> +  }

We should probably raise an error (or PR_LOG the problem) if the policy is null.

::: content/base/src/nsDocument.cpp
@@ +2803,5 @@
> +      // removed (see bug 949533)
> +#ifdef PR_LOGGING
> +      PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("%s %s %s",
> +           "This document has an old, x-content-security-policy",
> +           "header and the new parser doesn't support the non-standard",

s/parser/CSP implementation/

::: modules/libpref/src/init/all.js
@@ +1543,5 @@
>  
>  pref("security.csp.enable", true);
>  pref("security.csp.debug", false);
>  pref("security.csp.experimentalEnabled", false);
> +pref("security.csp.newbackend.enable", true);

nit: we should probably leave this preffed off for landing.  Set this to false.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_6_parser_tests_v2.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #17)
> Comment on attachment 8404140 [details] [diff] [review]
> part_6_parser_tests.patch

> Can you factor even more out of each test?  Each test finishes with a loop
> that checks the results; can you either make a macro generate that part (so
> the code only appears once) or a function?

Definitely can and of course did.

> Does it make any sense to move all the tests from test_csputils.js (xpcshell
> test) into this compiled code test?  There are a bunch in there.

I wrote a script that converts the tests from test_csputils.js over to fit our needs in the compiled code test suite. (see TestGoodGeneratedPolicies, and TestBadGeneratedPolicies). Since a 1:1 mapping of those tests is not possible anyway, I randomly applied directives from a pool of available directives where necessary. This conversion of tests was a good idea and really makes me comfortable that our parser can handle all the different cases.

> ::: content/base/test/TestCSPParser.cpp
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#ifndef MOZILLA_INTERNAL_API
> > +// some of the includes make use of internal string types
> 
> please add more to this comment: so we need to forward-declare some stuff.

Other test cases also only use this comment:
https://mxr.mozilla.org/mozilla-central/source/widget/tests/TestChromeMargin.cpp#20
https://mxr.mozilla.org/mozilla-central/source/widget/tests/TestWinTSF.cpp#28
And to be honest, I just copied it over from the other compiled code tests to make it work. Maybe someone can help who fully understands how these forward declarations make the compiler happy.

> @@ +57,5 @@
> > +  while (start != end) {
> > +    fprintf(stderr, "%c", *start++);
> > +  }
> > +  fprintf(stderr, "\n\n\n");
> > +}
> 
> Do the other compiled-code tests use fprintf, or is there another logging
> mechanism we should use?

That was still legacy code. Compiled code tests usually do not provide any logging at all.

> @@ +59,5 @@
> > +  }
> > +  fprintf(stderr, "\n\n\n");
> > +}
> > +
> > +const int CHECK_POLICY_AT_INDEX_0 = 0;
> 
> This is only used in one location, please just hard-code it and put a
> comment there to say why you're passing 0.

Refactored that part.

> @@ +74,5 @@
> > +  // create a CSP object
> > +  nsCOMPtr<nsIContentSecurityPolicy> csp =
> > +    do_CreateInstance(NS_CSPCONTEXT_CONTRACTID, &rv);
> > +  if (NS_FAILED(rv))
> > +    return rv;
> 
> NS_ENSURE_SUCCESS(rv, rv) works fine.  Or brace your if.

Updated that everywhere to use ENSURE_SUCCESS. Looks cleaner now.

> @@ +78,5 @@
> > +    return rv;
> > +
> > +  csp->SetRequestContext(selfURI, nullptr, nullptr, nullptr);
> > +  if (NS_FAILED(rv))
> > +    return rv;
> 
> Same here.  NS_ENSURE_SUCCESS(rv, rv);
> 
> @@ +99,5 @@
> > +    if (NS_FAILED(rv))
> > +      return rv;
> > +
> > +    if (actualPolicyCount != expectedPolicyCount)
> > +      return NS_ERROR_FAILURE;
> 
> NS_ENSURE_FALSE(actualPolicyCount == expectedPolicyCount, NS_ERROR_FAILURE);

Fixed, as mentioned above.

> @@ +147,5 @@
> > +    { "script-src 'sha256-siVR8vAcqP06h2ppeNwqgjr0yZ6yned4X2VF84j4GmI='",
> > +      "script-src 'sha256-siVR8vAcqP06h2ppeNwqgjr0yZ6yned4X2VF84j4GmI=';" }
> > +  };
> > +
> > +  uint32_t size = sizeof(policies) / sizeof(PolicyTest);
> 
> "size" is a weird name for this variable.  Maybe "policyCount" or something?

Indeed, size is weired, changed it.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 8409830 [details] [diff] [review]
part_1_backend_stubs_v2.patch

Drive by review comment:

+nsCSPContext::~nsCSPContext()
+{
+  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
+    if (mPolicies[i]) {
+      delete mPolicies[i];

You can remove the if check here, delete does that check for you.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_2_contentpolicy_tiein_v2.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #16)
> Comment on attachment 8405638 [details] [diff] [review]
> part_2_contentpolicy_tiein.patch
> 
> Review of attachment 8405638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good.  I'd like to see this one more time, though.
> 
> ::: content/base/src/nsCSPContext.cpp
> @@ +181,5 @@
> > +  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
> > +    if (!mPolicies[i]->allows(aContentType, NONCE, aNonce)) {
> > +      *shouldReportViolation = true;
> > +      if (!mPolicies[i]->getReportOnlyFlag())
> > +        *allowsNonce = false;
> 
> Are the semantics of allowsNonce and allowsHash the same as allowsEval and
> such?  If so, there *must* be a way to abstract out these loops; all the
> functions look the same.  Maybe via macro?

That's a good idea. Added a private member function GetAllowsInternal which abstracts that away.

> @@ +241,5 @@
> > +  }
> > +
> > +  NS_ASSERTION(mSelfURI, "No selfURI in SetRequestContext");
> > +
> > +  return NS_OK;
> 
> I assume the rest of the data gathering (referrer, channel, etc) will be
> done by future patches?

Indeed, added a comment to make it explicit.

 
> @@ +273,5 @@
> > +  // * Content Type is not whitelisted (CSP Reports, TYPE_DOCUMENT, etc).
> > +  // * Fast Path for Apps
> > +
> > +
> > +  // TODO(sid): implement report-only support
> 
> Please remove this TODO if report-only support is supported by this patch.

Cleaned the comments up everywehre in the file, including this one.

 
> @@ +286,5 @@
> > +  // nonce.
> > +  nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryInterface(aRequestContext);
> > +  bool noncePreload = doc && (aContentType == nsIContentPolicy::TYPE_SCRIPT ||
> > +                              aContentType == nsIContentPolicy::TYPE_STYLESHEET);
> > +  for (uint32_t p = 0; p < mPolicies.Length(); p++) {
> 
> This loop is going through the policies and making sure they *all* permit
> the load, right?

Yes, it's now in GetAllowsInternal.

> @@ +289,5 @@
> > +                              aContentType == nsIContentPolicy::TYPE_STYLESHEET);
> > +  for (uint32_t p = 0; p < mPolicies.Length(); p++) {
> > +    if (!mPolicies[p]->permits(aContentType, aContentLocation, violatedDirective)) {
> > +
> > +      if (!noncePreload) {
> 
> Please put a comment before this if block to summarize what it's doing.

Added comment.

 
> @@ +302,5 @@
> > +            bool allowsNonce = false;
> > +            GetAllowsNonce(nonce, aContentType, &shouldReportViolation, &allowsNonce);
> > +            if (allowsNonce) {
> > +              break;
> > +            }
> 
> if GetAllowsNonce returns an nsresult, please check it for errors.  Why do
> we break when nonce is allowed?  Do we stop checking the other policies or
> should this be "continue" instead of "break"?

Garret refactored that part completely, it's not in an permits function, which abstracts that part away and cleans it up really nicely.
He also added a very detailed comment on top of what is going on here.

> @@ +309,5 @@
> > +      }
> > +
> > +      // the policy does not allow the load, reject the load and report to console
> > +      if (!mPolicies[p]->getReportOnlyFlag())
> > +        *aDecision = nsIContentPolicy::REJECT_SERVER;
> 
> Nit: please brace the if.

I fixed that everywhere. No more if's without curlys since you statement in nsCSPUtils.cpp.

> 
> @@ +322,5 @@
> > +
> > +      observerService->NotifyObservers(aContentLocation,
> > +                                       CSP_VIOLATION_TOPIC,
> > +                                       violatedDirective.get());
> > +      // TODO(sid): console error reporting
> 
> For the violation reporting TODOs, please reference bug 994322 in the
> comment.

Put in forward referrals including bug numbers everywhere.

> @@ -199,5 @@
> > -                                     uint32_t flags,
> > -                                     nsIAsyncVerifyRedirectCallback *callback)
> > -{
> > -  return NS_ERROR_NOT_IMPLEMENTED;
> > -}
> 
> Since we're not implementing nsIChannelEventSink, please remove it from the
> backend_stubs patch too.

I guess I already deleted that part in the previous patch. Should be fine now.

> @@ +394,5 @@
> > +
> > +  // NOTE: the document instance that's deserializing this object (via its
> > +  // principal) should hook itself into this._principal manually.  If they
> > +  // don't, the CSP reports will likely be blocked by nsMixedContentBlocker.
> > +  return NS_OK;
> 
> Since we're not storing the principal, please remove this comment.

Did that.

> @@ +403,5 @@
> >  {
> > +  // need to serialize the context: request origin and such. They are
> > +  // used when sending reports.  Since _request and _requestOrigin are just
> > +  // different representations of the same thing, only save _requestOrigin
> > +  // (an nsIURI).
> 
> Please update this comment to reference the right class members (not
> _request and _requestOrigin)
> 
> @@ +411,5 @@
> > +                                               true);
> > +  if (NS_FAILED(rv)) {
> > +    fprintf(stderr, "OMG, failed to write mSelfURI");
> > +  }
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> Please remove this if block, it's unnecessary with the NS_ENSURE_SUCCESS.

Yes, deleted the fprintf including surrounding if.

> @@ +416,5 @@
> > +
> > +  // we can't serialize a reference to the principal that triggered this
> > +  // instance to serialize, so when this is deserialized by the principal the
> > +  // caller must hook it up manually by calling setRequestContext on this
> > +  // instance with the appropriate nsIChannel.
> 
> If we're not storing the principal anyway, please remove this comment or
> change it to say "some things aren't stored during serialization, they must
> be restored by calling SetRequestContext on this instance after
> deserialization".
> 
> ::: content/base/src/nsCSPUtils.cpp
> @@ +655,5 @@
> >    }
> >  
> > +  // if [frame-ancestors] is not listed explicitly then default to true
> > +  // without consulting [default-src]
> > +  // TODO: currently [frame-ancestors] is mapped to TPE_DOCUMENT (needs to be fixed)
> 
> Typo: TPE_DOCUMENT should be TYPE_DOCUMENT.  Also, do we know how to fix
> this or should we remove the TODO and instead put in:

Fixed that nit :-)

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #27)
> Comment on attachment 8409830 [details] [diff] [review]
> part_1_backend_stubs_v2.patch
> 
> Drive by review comment:
> 
> +nsCSPContext::~nsCSPContext()
> +{
> +  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
> +    if (mPolicies[i]) {
> +      delete mPolicies[i];
> 
> You can remove the if check here, delete does that check for you.

Oh, will fix that right away, didn't know that actually. Thanks Johnny!

Comment 30

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8412261 [details] [diff] [review]
part_2_contentpolicy_tiein_v2.patch

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

Mostly minor fixes.  r=me, please get grobinson's review too.

::: content/base/src/nsCSPContext.cpp
@@ +57,5 @@
> +#endif
> +
> +  nsresult rv = NS_OK;
> +
> +  // CSP Service (calls this) checks a number of things:

Please clarify that nsCSPService *already* completed the following list, and explain why it matters (e.g., we can assume aContentLocation is not null).

@@ +69,2 @@
>    *outDecision = nsIContentPolicy::ACCEPT;
> +  nsString violatedDirective;

Nit: please move this declaration down lower closer to where it's used.

@@ +87,5 @@
> +
> +  nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryInterface(aRequestContext);
> +  bool problematicPreload = doc &&
> +      (aContentType == nsIContentPolicy::TYPE_SCRIPT ||
> +       aContentType == nsIContentPolicy::TYPE_STYLESHEET);

Nit: line up boolean elements on the right hand side of the assignment operator.

bool x = foo &&
         (bar ||
          baz);

Also, please change problematicPreload to something like "isPreload"; it's only a problem for this implementation, not in general.  (It is not an error condition.)

@@ +115,5 @@
> +      }
> +
> +      // Do not send a report or notify observers if this is a preload - the
> +      // decision may be wrong due to the inability to get the nonce, and will
> +      // incorrectly fail the unit tests.

Does the preload phase cause loads that will eventually be disallowed, or only block things that should be allowed in the future?  If it is overly strict (more blocking than necessary), clarify that here, please.

@@ +154,5 @@
> +                  nsIClassInfo::MAIN_THREAD_ONLY,
> +                  NS_CSPCONTEXT_CID)
> +NS_IMPL_ISUPPORTS2_CI(nsCSPContext,
> +                      nsIContentSecurityPolicy,
> +                      nsISerializable)

ISUPPORTS2 => ISUPPORTS (see bug 900908).

@@ +162,5 @@
>  {
> +#ifdef PR_LOGGING
> +  if (!gCspContextPRLog) {
> +    gCspContextPRLog = PR_NewLogModule("CSPContext");
> +  }

Can you turn this into a lazy initialization globally, so we just call getCSPPRLogModule() or something?  Otherwise, we'll want to do the null check everywhere gCspContextPRLog is used.

@@ +190,5 @@
>  NS_IMETHODIMP
>  nsCSPContext::GetPolicy(uint32_t aIndex, nsAString& outStr)
>  {
>    if (aIndex >= mPolicies.Length()) {
> +    return NS_ERROR_ILLEGAL_VALUE;

Hm, we should probably move this NS_ERROR code to where the original if check was installed.  Is that the stubs patch?

@@ +226,5 @@
> +#endif
> +
> +  if (aSelfURI){
> +    // Do not provide aSelfURI for AppendPolicy, see bug 991474
> +    NS_WARNING("aSelfURI should be a nullptr in AppendPolicy and removed in bug 991474");

Please make this comment a little more explicit: "aSelfURI will be disregarded since we will remove it once we can easily change the interface.  See bug 991474."

@@ +230,5 @@
> +    NS_WARNING("aSelfURI should be a nullptr in AppendPolicy and removed in bug 991474");
> +  }
> +
> +  // Use the mSelfURI from setRequestContext, see bug 991474
> +  NS_ASSERTION(mSelfURI, "mSelfURI required for AppendPolicy");

This assertion should say what expectation was broken: "mSelfURI required in AppendPolicy, but is not set"

@@ +243,5 @@
> +nsCSPContext::getAllowsInternal(nsContentPolicyType aContentType,
> +                                enum CSPKeyword aKeyword,
> +                                const nsAString& aHashOrNonce,
> +                                bool* outShouldReportViolation,
> +                                bool* outDecision) const

Nit: name outDecision something that implies a boolean value (like "outIsAllowed").

@@ +253,5 @@
> +  if (aKeyword == CSP_NONCE || aKeyword == CSP_HASH) {
> +    if (!(aContentType == nsIContentPolicy::TYPE_SCRIPT ||
> +          aContentType == nsIContentPolicy::TYPE_STYLESHEET)) {
> +      *outDecision = false;
> +      return NS_OK;

Nit: please make the comment more useful (e.g., "skip things that aren't hash/nonce compatible").

@@ +349,5 @@
>                                  nsIPrincipal* aDocumentPrincipal,
>                                  nsIChannel* aChannel)
>  {
> +  if (!aSelfURI && !aChannel) {
> +    return NS_ERROR_FAILURE;

Please log an error here so it's easier to debug the failure.

@@ +355,5 @@
> +
> +  mSelfURI = aSelfURI;
> +
> +  if (!mSelfURI) {
> +    aChannel->GetURI(getter_AddRefs(mSelfURI));

obtain the nsresult and handle any errors.

@@ +358,5 @@
> +  if (!mSelfURI) {
> +    aChannel->GetURI(getter_AddRefs(mSelfURI));
> +  }
> +
> +  NS_ASSERTION(mSelfURI, "No mSelfURI in SetRequestContext");

Please make the assertion message more actionable.  Why do we need the mSelfURI?

@@ +438,5 @@
> +
> +  // We can't serialize a reference to the principal that triggered this
> +  // instance to serialize, so when this is deserialized by the principal the
> +  // caller must hook it up manually by calling setRequestContext on this
> +  // instance with the appropriate nsIChannel.

Maybe delete this comment since we don't use the principal anymore.

@@ +440,5 @@
> +  // instance to serialize, so when this is deserialized by the principal the
> +  // caller must hook it up manually by calling setRequestContext on this
> +  // instance with the appropriate nsIChannel.
> +
> +  // Finally, serialize all the policies.

Nit: "Finally" sounds awkward here.

Comment 31

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8410774 [details] [diff] [review]
part_6_parser_tests_v2.patch

Postponing review since it sounds like ckerschb is going to add more tests.  (Yay!)

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 8409829 [details] [diff] [review]
part_0_parser_utils_v3.patch

Finally found enough time to make it through this whole patch! This is looking pretty good, though I found some stuff we should fix before this lands (some of which I mentioned in person already, also listed below). Given that, I'm giving this an r-, because I want to look this over once more after you've dealt with this feedback.

- In nsCSPParser::nsCSPParser():

+ , mCurValue(NS_LITERAL_STRING(""))
+ , mCurToken(NS_LITERAL_STRING(""))

No need for these explicit initializers, nsString's default ctor does this for you.

- In nsCSPParser::consumeWhiteSpace():

+  while (!(mCurChar >= mEndChar) && accept(WHITESPACE)) {

This might read a bit easier as while (mCurChar < mEndChar && ...)

bool
+nsCSPParser::accept(char16_t aSymbol)
+{
+#ifdef PR_LOGGING
+  PR_LOG(gCspParserPRLog, PR_LOG_DEBUG,
+        ("nsCSPParser::accept; mCurChar %c == aSymbol %c", *mCurChar, aSymbol));
+#endif
+
+  return (*mCurChar == aSymbol) && advance();
+}

There's a few of these tiny helpers which I'd expect to be called quite a lot... maybe worth inlining?

+void
+nsCSPParser::resetCurValue()
+{
+  mCurValue = NS_LITERAL_STRING("");

mCurValue.Truncate() is probably few instructions less. Also probably worth inlining this one too.

- In nsCSPParser::subPath(nsCSPHostSrc* aCspHost):

+  if (!hostChar()) {
+    return false;
+  }
+  while (hostChar() || accept(UNDERLINE)) { /* consume */ }
+  if (accept(SLASH)) {
+    aCspHost->appendPath(mCurValue);
+    // Resetting current value since we are appending parts of the path
+    // to aCspHost, e.g; "http://www.example.com/path1/path2" then the
+    // first part is "/path1", second part "/path2"
+    resetCurValue();
+    if (peek(*mEndChar)) {
+      return true;
+    }
+    return subPath(aCspHost);

Do we need to protect against a malicious site sending us paths with a massive number of path segments here? If so, we should probably not have this be a recursive function to avoid exhausting the stack in a case like that.

- In nsCSPParser::subHost():

+  while (hostChar()) { /* consume */ }
+  if (accept(DOT)) {
+    if (!accept(isCharacterToken)) {
+      return false;
+    }
+    return subHost();

Same here.

- In nsCSPParser::host():

+  if (accept(WILDCARD)) {
+    // Might solely be the wildcard
+    if (peek(*mEndChar)) {
+      nsCSPHostSrc *cspHost = new nsCSPHostSrc(mCurValue);
+      return cspHost;

Might as well loose cspHost and just return new CSPHostSrc...

+  // Create a new nsCSPHostSrc with the parsed host.
+  nsCSPHostSrc *cspHost = new nsCSPHostSrc(mCurValue);
+  return cspHost;

ditto.

+// B2G uses special hosts; "app://{app-host-is-uid}""

app:// uri's are not specific to B2G, they can be used on android with the webrt etc too, I believe...

+nsCSPHostSrc*
>+nsCSPParser::hostB2G()

... so maybe rename this to appHost or somesuch (here and elsewhere).

+// nonce-source = "'nonce-" nonce-value "'"
+nsCSPNonceSource*
+nsCSPParser::nonceSource()

Maybe rename nsCSPNonceSource to nsCSPNonceSrc for consistency with the other nsCSP*Src classes? Same for nsCSPHashSource.

+  nsAutoString lowerExpr(mCurToken);
+  ToLowerCase(lowerExpr);
+
+  // Check if mCurToken begins with "'nonce-" and ends with "'"
+  if (!StringBeginsWith(lowerExpr, NS_ConvertUTF8toUTF16(CSP_EnumToKeyword(CSP_NONCE))) ||
+      lowerExpr.Last() != SINGLEQUOTE) {

You could do all of the above (i.e. save yourself a string copy) with something like this:

  if (!StringBeginsWith(mCurToken, NS_ConvertUTF8toUTF16(CSP_EnumToKeyword(CSP_NONCE)), nsASCIICaseInsensitiveStringComparator()),
      lowerExpr.Last() != SINGLEQUOTE) {

+  // Trim surrounding single quotes from original mCurToken, because
+  // the nonce-value itself is not supposed to be converted to lower case.
+  nsAutoString expr(mCurToken);
+  expr.Trim("'");

You could trim the first and last characters w/o doing a string copy with something like this:

  const nsAString& expr = Substring(mCurToken, 1, mCurToken.Length() - 2)

- In nsCSPParser::hashSource():

+  // Check if mCurToken starts and ends with "'"
+  if (mCurToken.First() != SINGLEQUOTE ||
+      mCurToken.Last() != SINGLEQUOTE) {
+    return nullptr;
+  }
+
+  // Trim surrounding single quotes
+  nsAutoString expr(mCurToken);
+  expr.Trim("'");

Same here, use Substring() and save a string copy.

+  nsAutoString algo(Substring(expr, 0, dashIndex));
+  nsAutoString hash(Substring(expr, dashIndex + 1, expr.Length() - dashIndex + 1));

Replace nsAutoString here with const nsAString&.

- In nsCSPParser::sourceExpression():

+  // Calling resetCurChar allows us to use mCurChar and mEndChar
+  // to parse mCurToken; e.g. mCurToken = "http://www.example.com", then
+  // mCurChar = 'h'
+  // mEndChar = 'm'

Per our discussion you should update this, and other similar comments to state that mEndChar actually points just past the last character.

+  // mCurValue = ""
+  resetCurChar(mCurToken);
+
+  // Check if mCurToken starts with a scheme
+  nsAutoString parsedScheme;
+  if (nsCSPSchemeSrc* cspScheme = schemeSource()) {
+    // mCurToken might only enforce a specific scheme
+    if (peek(*mEndChar)) {

Per our discussion we should change this pattern (appears earlier in this file as well) to calling an atEnd() (or whatever you want to call it) method that just checks if mCurChar points at mEndChar and that way avoid ever dereferencing mEndChar (which I think is safe in your code here, but conceptionally is not supposed to be done as it points to the end, and dereferencing it points past the actual string which may or may not be a zero terminator).

+    // If something follows the scheme, we do not create
+    // a nsCSPSchemeSrc, but rather a nsCSPHostSrc, which
+    // needs to know the scheme to enforce; remember the
+    // scheme and delete cspScheme;
+    cspScheme->toString(parsedScheme);
+    parsedScheme.Trim(":");

Maybe be explicit here about only trimming the ':' off the end here and call parsedScheme(":", false, true).

- In nsCSPParser::sourceList(nsTArray<nsCSPBaseSrc*>& outSrcs):

+    // mCurToken is only set here and remains the current token
+    // to be processed, which avoid passing arguments between functions.
+    mCurToken = mCurDir[i];
+
+#ifdef PR_LOGGING
+  PR_LOG(gCspParserPRLog, PR_LOG_DEBUG,
+        ("nsCSPParser::sourceList: %s", NS_ConvertUTF16toUTF8(mCurToken).get()));

Nit: 2 more spaces for indentation.

- In nsCSPParser::reportURIList(nsTArray<nsCSPBaseSrc*>& outSrcs):

+  for (uint32_t i = 1; i < mCurDir.Length(); i++) {
+    mCurToken = mCurDir[i];
+
+#ifdef PR_LOGGING
+  PR_LOG(gCspParserPRLog, PR_LOG_DEBUG,
+        ("nsCSPParser::reportURIList: %s", NS_ConvertUTF16toUTF8(mCurToken).get()));

Same nit :)

- In nsCSPParser.h:

+ * Continuing parsing, the parser consumes the next string of that array, resetting:
+ *   mCurToken = "http://www.example.com"
+ *                ^                    ^
+ *                mCurChar            mEndChar

Per our in person conversation, fix this to illustrate that mEndChar points *after* the 'm' at the end of the string. Same thing blow...

- In nsCSPUtil.cpp:

+void
+CSP_LogStrMessage(const nsAString& aMsg) {

Nit, opening curly brace on its own line.

+  nsCOMPtr<nsIConsoleService> console(do_GetService("@mozilla.org/consoleservice;1"));
+
+  if (!console) {
+    return;
+  }
+  console->LogStringMessage(ToNewUnicode(aMsg));

This will leak the string!

+CSP_CreateHostSrcFromURI(nsIURI* aURI) {

Nit, opening curly brace on its own line.

- In CSP_IsValidDirective():

+  uint32_t length = sizeof(CSPStrDirectives) / sizeof(CSPStrDirectives[0]);
+  for (uint32_t i = 0; i < length; i++) {

Could you just use CSP_LAST_DIRECTIVE_VALUE here instead of computing the length and statically assert (STATIC_ASSERT) that things are what's expected?

+bool
+CSP_IsQuotelessKeyword(const nsAString& aKey) {

Nit: Opening curly brace on its own line.

+  nsString lowerKey = PromiseFlatString(aKey);
+  ToLowerCase(lowerKey);

No need for the PromiseFlatString() call here.

+  uint32_t length = sizeof(CSPStrKeywords) / sizeof(CSPStrKeywords[0]);

Use CSP_LAST_KEYWORD_VALUE here and static assert?

+  for (uint32_t i = 0; i < length; i++) {
+    keyword.AssignASCII(CSPStrKeywords[i]);
+    keyword.Trim("'");

Maybe assign CSPStrKeywords[i] + 1 to keyword (cause you know the first character is always a single quote to avoid having to memmove the string in the trim call, and then you can just trim the trailing quote instead.

- In nsCSPSchemeSrc::nsCSPSchemeSrc(const nsAString& aScheme):

+  nsString scheme = PromiseFlatString(aScheme);
+  ToLowerCase(scheme);
+  mScheme = scheme;

No need for the "scheme" variable here, and the PromiseFlatString() should also not be needed (because you'll be mutating the string so a copy will be forced), just:

  mScheme = aScheme;
  ToLowerCase(mScheme);

And in fact you could then make this use a member initializer to avoid that running in addition to the assignment, i.e.:

nsCSPSchemeSrc::nsCSPSchemeSrc(const nsAString& aScheme)
  : mScheme(aScheme)
{
  ToLowerCase(mScheme);
}

Same thing in nsCSPHostSrc.

- In nsCSPHostSrc::permits():

+  // Extract the host part from aUri.
+  nsAutoCString aUriHost;
+  rv = aUri->GetHost(aUriHost);

Please rename aUriHost uriHost, the 'a' prefix is fairly well reserved for argument names, and using it elsewhere in gecko code tends to be very confusing to readers.

+  // Check it the allowed host starts with a wilcard.
+  if (mHost.First() == '*') {
+    // Eliminate leading "*." and check if uriHost ends with defined mHost.
+    nsString wildCardHost = mHost;
+    wildCardHost = Substring(wildCardHost, 2, wildCardHost.Length() - 2);

Maybe assert here that the second character really is a '.'?

+  // Otherwise compare the ports
+  else {
+    nsString portStr = NS_LITERAL_STRING("");
+    portStr.AppendInt(uriPort);
+    if (!mPort.Equals(portStr)) {
+      return false;

I wonder whether it would make more sense to make mPort be stored as an integer instead of a string and thus avoid having to stringify in places like this?

- In nsCSPHostSrc::setScheme(const nsAString& aScheme) and a bunch of the following methods:

+  nsString scheme = PromiseFlatString(aScheme);
+  ToLowerCase(scheme);
+  mScheme = scheme;

No need for the PromiseFlatString(), just assign into mScheme and call ToLowerCase(mScheme).

- In nsCSPHashSource::nsCSPHashSource(const nsAString& aAlgo, const nsAString& aHash):

+  nsString algo = PromiseFlatString(aAlgo);
+  ToLowerCase(algo);
+  mAlgorithm = algo;

Ditto.

- In nsCSPDirective::~nsCSPDirective()

+  for (uint32_t i = 0; i < mSrcs.Length(); i++) {
+    if (mSrcs[i]) {
+      delete mSrcs[i];

Remove the unnecessary if check :)

+bool
+nsCSPDirective::isDefaultDirective() const {

Opening curly brace on its own line.

- In nsCSPPolicy::~nsCSPPolicy():

+    if (mDirectives[i]) {
+      delete mDirectives[i];

Remove the if check.

- In CSP_DirectiveToEnum(const nsAString& aDir):

+  nsString lowerDir = PromiseFlatString(aDir);
+  ToLowerCase(lowerDir);

No need for the PromiseFlatString()

- In CSP_KeywordToEnum(const nsAString& aKey):

+  nsString lowerKey = PromiseFlatString(aKey);
+  ToLowerCase(lowerKey);

Same here.

Comment 33

[Garrett Robinson [:grobinson]]

Comment on attachment 8412261 [details] [diff] [review]
part_2_contentpolicy_tiein_v2.patch

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

::: content/base/src/nsCSPContext.cpp
@@ +69,2 @@
>    *outDecision = nsIContentPolicy::ACCEPT;
> +  nsString violatedDirective;

Nit: local variable should be nsAutoString

@@ +88,5 @@
> +  nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryInterface(aRequestContext);
> +  bool problematicPreload = doc &&
> +      (aContentType == nsIContentPolicy::TYPE_SCRIPT ||
> +       aContentType == nsIContentPolicy::TYPE_STYLESHEET);
> +

Disagree with Sid's point - this variable indicates that the load is a) a preload and b) would be adversely affected if we did not use the special logic (because it is script or style, and can be nonced). isPreload is not accurate, and cannot be consistently determined (the QI to nsIDOMHTMLDocument does not work for all load types).

@@ +115,5 @@
> +      }
> +
> +      // Do not send a report or notify observers if this is a preload - the
> +      // decision may be wrong due to the inability to get the nonce, and will
> +      // incorrectly fail the unit tests.

Note: the preload phase only blocks things that should be allowed in the future. Since we don't have the nonce, we will default to blocking any resources that would otherwise be whitelisted by the nonce.

@@ +162,5 @@
>  {
> +#ifdef PR_LOGGING
> +  if (!gCspContextPRLog) {
> +    gCspContextPRLog = PR_NewLogModule("CSPContext");
> +  }

e.g. http://dxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSiteSecurityService.cpp#38

@@ +241,5 @@
>  
>  NS_IMETHODIMP
> +nsCSPContext::getAllowsInternal(nsContentPolicyType aContentType,
> +                                enum CSPKeyword aKeyword,
> +                                const nsAString& aHashOrNonce,

Nit: this name is a little confusing. The string here is not actually the hash, but the content of the element to be hashed (because we don't know what algorithm to use for hashing until we're inside the policy). I used to call it aContent but that wasn't great. aHashOrNonce might be ok, although maybe you should add a comment to this effect.

@@ +279,4 @@
>  {
> +  return getAllowsInternal(nsIContentPolicy::TYPE_SCRIPT,
> +                           CSP_UNSAFE_INLINE,
> +                           NS_LITERAL_STRING(""),

Nit: I think EmptyString() is preferred here

Comment 34

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #33)
> Disagree with Sid's point - this variable indicates that the load is a) a
> preload and b) would be adversely affected if we did not use the special
> logic (because it is script or style, and can be nonced). isPreload is not
> accurate, and cannot be consistently determined (the QI to
> nsIDOMHTMLDocument does not work for all load types).

Ah, ok.  Can we find a better name though? Perhaps something like "skipPreload" or "preloadWithoutContext"?  "problematicPreload" makes me think we're checking an error condition (it's not an error, it's a preload) and I have to read the comment above it to determine what the value of this variable indicates.  

> @@ +162,5 @@
> >  {
> > +#ifdef PR_LOGGING
> > +  if (!gCspContextPRLog) {
> > +    gCspContextPRLog = PR_NewLogModule("CSPContext");
> > +  }
> 
> e.g.
> http://dxr.mozilla.org/mozilla-central/source/security/manager/boot/src/
> nsSiteSecurityService.cpp#38

For context if you are reading this as a bug comment, this is in reply to my comment "Can you turn this into a lazy initialization globally, so we just call getCSPPRLogModule() or something? Otherwise, we'll want to do the null check everywhere gCspContextPRLog is used."

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_0_parser_utils_v4.patch

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #32)
> Comment on attachment 8409829 [details] [diff] [review]
> part_0_parser_utils_v3.patch
> 
> Finally found enough time to make it through this whole patch! This is
> looking pretty good, though I found some stuff we should fix before this
> lands (some of which I mentioned in person already, also listed below).
> Given that, I'm giving this an r-, because I want to look this over once
> more after you've dealt with this feedback.

Thanks Johnny for looking this over; I guess I incorporated all of your suggestions which definitely improved the code and also the readiability of the parser and the utils. The only thing I was not able to incorporate was the complete elimination of PromiseFlatString :-( Please let me know if/how I can avoid usage of such.
 
> - In nsCSPParser::nsCSPParser():
> 
> + , mCurValue(NS_LITERAL_STRING(""))
> + , mCurToken(NS_LITERAL_STRING(""))
> 
> No need for these explicit initializers, nsString's default ctor does this
> for you.

I generally like all member variables to be initialized explicitly using the argument list because one can immediately see what the members default value is by looking at the ctor code. Anyway, I understand your point of view and went ahead and did the change.
 
> - In nsCSPParser::consumeWhiteSpace():
> 
> +  while (!(mCurChar >= mEndChar) && accept(WHITESPACE)) {
> 
> This might read a bit easier as while (mCurChar < mEndChar && ...)

Totally, no need to make this so complicated.
 
> bool
> +nsCSPParser::accept(char16_t aSymbol)
> +{
> +#ifdef PR_LOGGING
> +  PR_LOG(gCspParserPRLog, PR_LOG_DEBUG,
> +        ("nsCSPParser::accept; mCurChar %c == aSymbol %c", *mCurChar,
> aSymbol));
> +#endif
> +
> +  return (*mCurChar == aSymbol) && advance();
> +}
> 
> There's a few of these tiny helpers which I'd expect to be called quite a
> lot... maybe worth inlining?

I agree, this reads way better inlined, I inlined: advance, (2x) accept, (2x) peek, resetCurValue, atEnd().
 
> +void
> +nsCSPParser::resetCurValue()
> +{
> +  mCurValue = NS_LITERAL_STRING("");
> 
> mCurValue.Truncate() is probably few instructions less. Also probably worth
> inlining this one too.

Probably is, thanks for the info.
 
> - In nsCSPParser::subPath(nsCSPHostSrc* aCspHost):
> 
> +  if (!hostChar()) {
> +    return false;
> +  }
> +  while (hostChar() || accept(UNDERLINE)) { /* consume */ }
> +  if (accept(SLASH)) {
> +    aCspHost->appendPath(mCurValue);
> +    // Resetting current value since we are appending parts of the path
> +    // to aCspHost, e.g; "http://www.example.com/path1/path2" then the
> +    // first part is "/path1", second part "/path2"
> +    resetCurValue();
> +    if (peek(*mEndChar)) {
> +      return true;
> +    }
> +    return subPath(aCspHost);
> 
> Do we need to protect against a malicious site sending us paths with a
> massive number of path segments here? If so, we should probably not have
> this be a recursive function to avoid exhausting the stack in a case like
> that.

I don't think it's going to be a problem, but to be on the safe side, I rewrote subPath to use a while-loop and also put in an emergency exit, exiting the loop in case the host is longer than 512 chars. I think that's reasonable.
 
> - In nsCSPParser::subHost():
> 
> +  while (hostChar()) { /* consume */ }
> +  if (accept(DOT)) {
> +    if (!accept(isCharacterToken)) {
> +      return false;
> +    }
> +    return subHost();
> 
> Same here.

Same fix as above, also using the 512 character cutoff.
 
> - In nsCSPParser::host():
> 
> +  if (accept(WILDCARD)) {
> +    // Might solely be the wildcard
> +    if (peek(*mEndChar)) {
> +      nsCSPHostSrc *cspHost = new nsCSPHostSrc(mCurValue);
> +      return cspHost;
> 
> Might as well loose cspHost and just return new CSPHostSrc...

Already got that fixed before you looked over it, just hasn't made it to the review-patch yet (back then).
 
> +  // Create a new nsCSPHostSrc with the parsed host.
> +  nsCSPHostSrc *cspHost = new nsCSPHostSrc(mCurValue);
> +  return cspHost;
> 
> ditto.

Yep, done and done.

> 
> +// B2G uses special hosts; "app://{app-host-is-uid}""
> 
> app:// uri's are not specific to B2G, they can be used on android with the
> webrt etc too, I believe...
> 
> +nsCSPHostSrc*
> >+nsCSPParser::hostB2G()
> 
> ... so maybe rename this to appHost or somesuch (here and elsewhere).

Oh, I thought it's B2G specific. Renamed it to appHost(); (makes more sense anyway :-) ).
 
> +// nonce-source = "'nonce-" nonce-value "'"
> +nsCSPNonceSource*
> +nsCSPParser::nonceSource()
> 
> Maybe rename nsCSPNonceSource to nsCSPNonceSrc for consistency with the
> other nsCSP*Src classes? Same for nsCSPHashSource.

Ok, now it's consistent. The Parser uses 'Source' postfixes (like in the ABNF), and the utils uses 'Src' postfixes.
 
> +  nsAutoString lowerExpr(mCurToken);
> +  ToLowerCase(lowerExpr);
> +
> +  // Check if mCurToken begins with "'nonce-" and ends with "'"
> +  if (!StringBeginsWith(lowerExpr,
> NS_ConvertUTF8toUTF16(CSP_EnumToKeyword(CSP_NONCE))) ||
> +      lowerExpr.Last() != SINGLEQUOTE) {
> 
> You could do all of the above (i.e. save yourself a string copy) with
> something like this:
> 
>   if (!StringBeginsWith(mCurToken,
> NS_ConvertUTF8toUTF16(CSP_EnumToKeyword(CSP_NONCE)),
> nsASCIICaseInsensitiveStringComparator()),
>       lowerExpr.Last() != SINGLEQUOTE) {

Makes sense, rewrote that part.
 
> +  // Trim surrounding single quotes from original mCurToken, because
> +  // the nonce-value itself is not supposed to be converted to lower case.
> +  nsAutoString expr(mCurToken);
> +  expr.Trim("'");
> 
> You could trim the first and last characters w/o doing a string copy with
> something like this:
> 
>   const nsAString& expr = Substring(mCurToken, 1, mCurToken.Length() - 2)

Saving another string copy here then, awesome.
 
> - In nsCSPParser::hashSource():
> 
> +  // Check if mCurToken starts and ends with "'"
> +  if (mCurToken.First() != SINGLEQUOTE ||
> +      mCurToken.Last() != SINGLEQUOTE) {
> +    return nullptr;
> +  }
> +
> +  // Trim surrounding single quotes
> +  nsAutoString expr(mCurToken);
> +  expr.Trim("'");
> 
> Same here, use Substring() and save a string copy.

Yep, done that.
 
> +  nsAutoString algo(Substring(expr, 0, dashIndex));
> +  nsAutoString hash(Substring(expr, dashIndex + 1, expr.Length() -
> dashIndex + 1));
> 
> Replace nsAutoString here with const nsAString&.

Updated, but nsAString& does not work here:

 0:12.05 /home/ckerschb/moz/mc/content/base/src/nsCSPParser.cpp:558:14: error: non-const lvalue reference to type 'nsAString_internal' cannot bind to a temporary of type 'const nsDependentSubstring'
 0:12.05   nsAString& algo(Substring(expr, 0, dashIndex));


> - In nsCSPParser::sourceExpression():
> 
> +  // Calling resetCurChar allows us to use mCurChar and mEndChar
> +  // to parse mCurToken; e.g. mCurToken = "http://www.example.com", then
> +  // mCurChar = 'h'
> +  // mEndChar = 'm'
> 
> Per our discussion you should update this, and other similar comments to
> state that mEndChar actually points just past the last character.

Fixed, and updated comments everywhere.

> +  // mCurValue = ""
> +  resetCurChar(mCurToken);
> +
> +  // Check if mCurToken starts with a scheme
> +  nsAutoString parsedScheme;
> +  if (nsCSPSchemeSrc* cspScheme = schemeSource()) {
> +    // mCurToken might only enforce a specific scheme
> +    if (peek(*mEndChar)) {
> 
> Per our discussion we should change this pattern (appears earlier in this
> file as well) to calling an atEnd() (or whatever you want to call it) method
> that just checks if mCurChar points at mEndChar and that way avoid ever
> dereferencing mEndChar (which I think is safe in your code here, but
> conceptionally is not supposed to be done as it points to the end, and
> dereferencing it points past the actual string which may or may not be a
> zero terminator).

That was an awesome catch, you really must do a lot of code reviews - thanks for pointing this one out.
Immediately fixed that of course.
 
> +    // If something follows the scheme, we do not create
> +    // a nsCSPSchemeSrc, but rather a nsCSPHostSrc, which
> +    // needs to know the scheme to enforce; remember the
> +    // scheme and delete cspScheme;
> +    cspScheme->toString(parsedScheme);
> +    parsedScheme.Trim(":");
> 
> Maybe be explicit here about only trimming the ':' off the end here and call
> parsedScheme(":", false, true).

I assume, you mean >> parsedScheme.Trim(":", false, true);
Otherwise, let me know.
 
> - In nsCSPParser::sourceList(nsTArray<nsCSPBaseSrc*>& outSrcs):
> 
> +    // mCurToken is only set here and remains the current token
> +    // to be processed, which avoid passing arguments between functions.
> +    mCurToken = mCurDir[i];
> +
> +#ifdef PR_LOGGING
> +  PR_LOG(gCspParserPRLog, PR_LOG_DEBUG,
> +        ("nsCSPParser::sourceList: %s",
> NS_ConvertUTF16toUTF8(mCurToken).get()));
> 
> Nit: 2 more spaces for indentation.

Fixed that everywhere.
 
> - In nsCSPParser::reportURIList(nsTArray<nsCSPBaseSrc*>& outSrcs):
> 
> +  for (uint32_t i = 1; i < mCurDir.Length(); i++) {
> +    mCurToken = mCurDir[i];
> +
> +#ifdef PR_LOGGING
> +  PR_LOG(gCspParserPRLog, PR_LOG_DEBUG,
> +        ("nsCSPParser::reportURIList: %s",
> NS_ConvertUTF16toUTF8(mCurToken).get()));
> 
> Same nit :)

As mentioned above, fixed indendation for PR_LOGGING to be consistent everywhere.
 
> - In nsCSPParser.h:
> 
> + * Continuing parsing, the parser consumes the next string of that array,
> resetting:
> + *   mCurToken = "http://www.example.com"
> + *                ^                    ^
> + *                mCurChar            mEndChar
> 
> Per our in person conversation, fix this to illustrate that mEndChar points
> *after* the 'm' at the end of the string. Same thing blow...
> 
> - In nsCSPUtil.cpp:
> 
> +void
> +CSP_LogStrMessage(const nsAString& aMsg) {
> 
> Nit, opening curly brace on its own line.

Sure thing.
 
> +  nsCOMPtr<nsIConsoleService>
> console(do_GetService("@mozilla.org/consoleservice;1"));
> +
> +  if (!console) {
> +    return;
> +  }
> +  console->LogStringMessage(ToNewUnicode(aMsg));
> 
> This will leak the string!

Using a temporary, because we can not call .get() on an nsAString.

> +CSP_CreateHostSrcFromURI(nsIURI* aURI) {
> 
> Nit, opening curly brace on its own line.

Fixed.
 
> - In CSP_IsValidDirective():
> 
> +  uint32_t length = sizeof(CSPStrDirectives) / sizeof(CSPStrDirectives[0]);
> +  for (uint32_t i = 0; i < length; i++) {
> 
> Could you just use CSP_LAST_DIRECTIVE_VALUE here instead of computing the
> length and statically assert (STATIC_ASSERT) that things are what's expected?

I can use CSP_LAST_DIRECTIVE and use and NS_ASSERTION.
 
> +bool
> +CSP_IsQuotelessKeyword(const nsAString& aKey) {
> 
> Nit: Opening curly brace on its own line.

Finally I got it, so far member functions had the curly brace on it's on line and functions bound to the scope of a file had the opening brace in the same line. I fixed that everywhere now to be consistent.
 
> +  nsString lowerKey = PromiseFlatString(aKey);
> +  ToLowerCase(lowerKey);
> 
> No need for the PromiseFlatString() call here.

Really? How do I do that? I tried, compiler complains:

 0:16.63 In file included from /home/ckerschb/moz/mc-obj-dbg/content/base/src/Unified_cpp_content_base_src1.cpp:197:
 0:16.63 /home/ckerschb/moz/mc/content/base/src/nsCSPUtils.cpp:153:12: error: no viable conversion from 'const nsAString_internal' to 'nsString'
 0:16.63   nsString lowerKey = aKey;
 0:16.63            ^          ~~~~
 0:16.63 ../../../dist/include/nsTString.h:51:3: note: candidate constructor not viable: cannot bind base class object of type 'const nsAString_internal' to derived class reference 'const self_type &' (aka 'const nsString &') for 1st argument
 0:16.63   nsTString_CharT( const self_type& str )
 0:16.63   ^
 0:16.64 ../../../dist/include/string-template-def-unichar.h:11:45: note: expanded from macro 'nsTString_CharT'
 0:16.64 #define nsTString_CharT                     nsString
 0:16.64                                             ^
 0:16.64 ../../../dist/include/nsTString.h:57:3: note: candidate constructor not viable: no known conversion from 'const nsAString_internal' to 'const substring_tuple_type &' (aka 'const nsSubstringTuple &') for 1st argument
 0:16.64   nsTString_CharT( const substring_tuple_type& tuple )
 0:16.64   ^
 0:16.64 ../../../dist/include/string-template-def-unichar.h:11:45: note: expanded from macro 'nsTString_CharT'
 0:16.64 #define nsTString_CharT                     nsString
 0:16.64                                             ^
 0:16.64 1 error generated.
 
> +  uint32_t length = sizeof(CSPStrKeywords) / sizeof(CSPStrKeywords[0]);
> 
> Use CSP_LAST_KEYWORD_VALUE here and static assert?

As above, fixed that.
 
> +  for (uint32_t i = 0; i < length; i++) {
> +    keyword.AssignASCII(CSPStrKeywords[i]);
> +    keyword.Trim("'");
> 
> Maybe assign CSPStrKeywords[i] + 1 to keyword (cause you know the first
> character is always a single quote to avoid having to memmove the string in
> the trim call, and then you can just trim the trailing quote instead.

That makes sense, did that and added a comment.
 
> - In nsCSPSchemeSrc::nsCSPSchemeSrc(const nsAString& aScheme):
> 
> +  nsString scheme = PromiseFlatString(aScheme);
> +  ToLowerCase(scheme);
> +  mScheme = scheme;
> 
> No need for the "scheme" variable here, and the PromiseFlatString() should
> also not be needed (because you'll be mutating the string so a copy will be
> forced), just:
> 
>   mScheme = aScheme;
>   ToLowerCase(mScheme);

OK, works for me.
 
> And in fact you could then make this use a member initializer to avoid that
> running in addition to the assignment, i.e.:
> 
> nsCSPSchemeSrc::nsCSPSchemeSrc(const nsAString& aScheme)
>   : mScheme(aScheme)
> {
>   ToLowerCase(mScheme);
> }

Ah, I like that. Using the initalization list is what I wanted in the first place, but I thought I am forced to use PromiseFlatString. Thanks!
 
> Same thing in nsCSPHostSrc.

Fixe that consistently.
 
> - In nsCSPHostSrc::permits():
> 
> +  // Extract the host part from aUri.
> +  nsAutoCString aUriHost;
> +  rv = aUri->GetHost(aUriHost);
> 
> Please rename aUriHost uriHost, the 'a' prefix is fairly well reserved for
> argument names, and using it elsewhere in gecko code tends to be very
> confusing to readers.

Interesting how that ended up that way. Usually I try to be really consistent about 'a' and 'out' prefixes when naming variables/arguments. Fixed that of course.

> 
> +  // Check it the allowed host starts with a wilcard.
> +  if (mHost.First() == '*') {
> +    // Eliminate leading "*." and check if uriHost ends with defined mHost.
> +    nsString wildCardHost = mHost;
> +    wildCardHost = Substring(wildCardHost, 2, wildCardHost.Length() - 2);
> 
> Maybe assert here that the second character really is a '.'?

Indeed a good idea, put one there.
 
> +  // Otherwise compare the ports
> +  else {
> +    nsString portStr = NS_LITERAL_STRING("");
> +    portStr.AppendInt(uriPort);
> +    if (!mPort.Equals(portStr)) {
> +      return false;
> 
> I wonder whether it would make more sense to make mPort be stored as an
> integer instead of a string and thus avoid having to stringify in places
> like this?

I asked me that question when I implemented it, but since mPort might also be the wildcard ("*") character I though it's the better idea to stringify, otherwise we would need special case handling for this. Either way it's a tradeoff, if you rather have special case handling, let me know. I think the better solution is to stringify.
 
> - In nsCSPHostSrc::setScheme(const nsAString& aScheme) and a bunch of the
> following methods:
> 
> +  nsString scheme = PromiseFlatString(aScheme);
> +  ToLowerCase(scheme);
> +  mScheme = scheme;
> 
> No need for the PromiseFlatString(), just assign into mScheme and call
> ToLowerCase(mScheme).

Fixed that part up.
 
> - In nsCSPHashSource::nsCSPHashSource(const nsAString& aAlgo, const
> nsAString& aHash):
> 
> +  nsString algo = PromiseFlatString(aAlgo);
> +  ToLowerCase(algo);
> +  mAlgorithm = algo;
> 
> Ditto.

ditto :-)
 
> - In nsCSPDirective::~nsCSPDirective()
> 
> +  for (uint32_t i = 0; i < mSrcs.Length(); i++) {
> +    if (mSrcs[i]) {
> +      delete mSrcs[i];
> 
> Remove the unnecessary if check :)

Already did that when you pointed that out in your 'drive-by-review' comment :-)
 
> +bool
> +nsCSPDirective::isDefaultDirective() const {
> 
> Opening curly brace on its own line.

Inlined that function, and some other short ones.
 
> - In nsCSPPolicy::~nsCSPPolicy():
> 
> +    if (mDirectives[i]) {
> +      delete mDirectives[i];
> 
> Remove the if check.

Fixed.
 
> - In CSP_DirectiveToEnum(const nsAString& aDir):
> 
> +  nsString lowerDir = PromiseFlatString(aDir);
> +  ToLowerCase(lowerDir);
> 
> No need for the PromiseFlatString()

Nope, doesn't work!
 
> - In CSP_KeywordToEnum(const nsAString& aKey):
> 
> +  nsString lowerKey = PromiseFlatString(aKey);
> +  ToLowerCase(lowerKey);
> 
> Same here.

Same here, does not work :-)

As explained at the very top, I am not sure if we can eliminate PromiseFlatSTring everywhere. If I am mistaken, please let me know how to fix it and I will go ahead and do that immediately.

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_1_backend_stubs_v3.patch

Carrying over r+ from sstamm and grobinson.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #25)
> Comment on attachment 8409830 [details] [diff] [review]
> part_1_backend_stubs_v2.patch
> 
> Review of attachment 8409830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There was probably no need to flag me again because I gave you r+ in the
> last review of this patch, but I looked anyway.  Just a few minors.  r=me
> 
> ::: content/base/src/nsCSPContext.cpp
> @@ +120,5 @@
> > +  NS_ASSERTION(aSelfURI, "aSelfURI required for AppendPolicy");
> > +  nsCSPPolicy* policy = nsCSPParser::parseContentSecurityPolicy(aPolicyString, aSelfURI, aReportOnly, 0);
> > +  if (policy) {
> > +    mPolicies.AppendElement(policy);
> > +  }
> 
> We should probably raise an error (or PR_LOG the problem) if the policy is
> null.

PR_LOGGING is not yet available in that patch. But once it is introduced in one of the next patches I will definitely put a logging message there.
 
> ::: content/base/src/nsDocument.cpp
> @@ +2803,5 @@
> > +      // removed (see bug 949533)
> > +#ifdef PR_LOGGING
> > +      PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("%s %s %s",
> > +           "This document has an old, x-content-security-policy",
> > +           "header and the new parser doesn't support the non-standard",
> 
> s/parser/CSP implementation/

Updated.
 
> ::: modules/libpref/src/init/all.js
> @@ +1543,5 @@
> >  
> >  pref("security.csp.enable", true);
> >  pref("security.csp.debug", false);
> >  pref("security.csp.experimentalEnabled", false);
> > +pref("security.csp.newbackend.enable", true);
> 
> nit: we should probably leave this preffed off for landing.  Set this to
> false.

Good idea, did that.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

> > - In CSP_IsValidDirective():
> > 
> > +  uint32_t length = sizeof(CSPStrDirectives) / sizeof(CSPStrDirectives[0]);
> > +  for (uint32_t i = 0; i < length; i++) {
> > 
> > Could you just use CSP_LAST_DIRECTIVE_VALUE here instead of computing the
> > length and statically assert (STATIC_ASSERT) that things are what's expected?
> 
> I can use CSP_LAST_DIRECTIVE and use and NS_ASSERTION.

Apparently this code pattern also shows up in the .h file. Must have slipped through the cracks when doing the update - I will incorporate the same change for CSP_KeywordToEnum and others in the .h file once I prepare this patch for uploading.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_2_contentpolicy_tiein_v3.patch

Carrying over r+ from sstamm and grobinson.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #30)
> Comment on attachment 8412261 [details] [diff] [review]
> part_2_contentpolicy_tiein_v2.patch
> 
> Review of attachment 8412261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly minor fixes.  r=me, please get grobinson's review too.
> 
> ::: content/base/src/nsCSPContext.cpp
> @@ +57,5 @@
> > +#endif
> > +
> > +  nsresult rv = NS_OK;
> > +
> > +  // CSP Service (calls this) checks a number of things:
> 
> Please clarify that nsCSPService *already* completed the following list, and
> explain why it matters (e.g., we can assume aContentLocation is not null).

Expanded comment here.
 
> @@ +69,2 @@
> >    *outDecision = nsIContentPolicy::ACCEPT;
> > +  nsString violatedDirective;
> 
> Nit: please move this declaration down lower closer to where it's used.

Moved further down; also using nsAutoString.
 
> @@ +87,5 @@
> > +
> > +  nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryInterface(aRequestContext);
> > +  bool problematicPreload = doc &&
> > +      (aContentType == nsIContentPolicy::TYPE_SCRIPT ||
> > +       aContentType == nsIContentPolicy::TYPE_STYLESHEET);
> 
> Nit: line up boolean elements on the right hand side of the assignment
> operator.
> 
> bool x = foo &&
>          (bar ||
>           baz);

Did that as well.
 
> Also, please change problematicPreload to something like "isPreload"; it's
> only a problem for this implementation, not in general.  (It is not an error
> condition.)

Changed that to isPreload().
 
> @@ +115,5 @@
> > +      }
> > +
> > +      // Do not send a report or notify observers if this is a preload - the
> > +      // decision may be wrong due to the inability to get the nonce, and will
> > +      // incorrectly fail the unit tests.
> 
> Does the preload phase cause loads that will eventually be disallowed, or
> only block things that should be allowed in the future?  If it is overly
> strict (more blocking than necessary), clarify that here, please.

See grobinsons comment, 33.
 
> @@ +154,5 @@
> > +                  nsIClassInfo::MAIN_THREAD_ONLY,
> > +                  NS_CSPCONTEXT_CID)
> > +NS_IMPL_ISUPPORTS2_CI(nsCSPContext,
> > +                      nsIContentSecurityPolicy,
> > +                      nsISerializable)
> 
> ISUPPORTS2 => ISUPPORTS (see bug 900908).

Yes, double checked. We already fixed that back then.
 
> @@ +162,5 @@
> >  {
> > +#ifdef PR_LOGGING
> > +  if (!gCspContextPRLog) {
> > +    gCspContextPRLog = PR_NewLogModule("CSPContext");
> > +  }
> 
> Can you turn this into a lazy initialization globally, so we just call
> getCSPPRLogModule() or something?  Otherwise, we'll want to do the null
> check everywhere gCspContextPRLog is used.

Yes, that was a good idea. Way cleaner now.
 
> @@ +190,5 @@
> >  NS_IMETHODIMP
> >  nsCSPContext::GetPolicy(uint32_t aIndex, nsAString& outStr)
> >  {
> >    if (aIndex >= mPolicies.Length()) {
> > +    return NS_ERROR_ILLEGAL_VALUE;
> 
> Hm, we should probably move this NS_ERROR code to where the original if
> check was installed.  Is that the stubs patch?

Probably we should have done that, but since all these patches are going to land at the same time it doesn't make a huge difference by the end of the day.
 
> @@ +226,5 @@
> > +#endif
> > +
> > +  if (aSelfURI){
> > +    // Do not provide aSelfURI for AppendPolicy, see bug 991474
> > +    NS_WARNING("aSelfURI should be a nullptr in AppendPolicy and removed in bug 991474");
> 
> Please make this comment a little more explicit: "aSelfURI will be
> disregarded since we will remove it once we can easily change the interface.
> See bug 991474."

Updated.
 
> @@ +230,5 @@
> > +    NS_WARNING("aSelfURI should be a nullptr in AppendPolicy and removed in bug 991474");
> > +  }
> > +
> > +  // Use the mSelfURI from setRequestContext, see bug 991474
> > +  NS_ASSERTION(mSelfURI, "mSelfURI required for AppendPolicy");
> 
> This assertion should say what expectation was broken: "mSelfURI required in
> AppendPolicy, but is not set"

Updated.
 
> @@ +243,5 @@
> > +nsCSPContext::getAllowsInternal(nsContentPolicyType aContentType,
> > +                                enum CSPKeyword aKeyword,
> > +                                const nsAString& aHashOrNonce,
> > +                                bool* outShouldReportViolation,
> > +                                bool* outDecision) const
> 
> Nit: name outDecision something that implies a boolean value (like
> "outIsAllowed").

Makes sense - updated, also renamed aHashOrNonce to aNonceOrContent.
 
> @@ +253,5 @@
> > +  if (aKeyword == CSP_NONCE || aKeyword == CSP_HASH) {
> > +    if (!(aContentType == nsIContentPolicy::TYPE_SCRIPT ||
> > +          aContentType == nsIContentPolicy::TYPE_STYLESHEET)) {
> > +      *outDecision = false;
> > +      return NS_OK;
> 
> Nit: please make the comment more useful (e.g., "skip things that aren't
> hash/nonce compatible").

Updated.
 
> @@ +349,5 @@
> >                                  nsIPrincipal* aDocumentPrincipal,
> >                                  nsIChannel* aChannel)
> >  {
> > +  if (!aSelfURI && !aChannel) {
> > +    return NS_ERROR_FAILURE;
> 
> Please log an error here so it's easier to debug the failure.

Makes sense. Updated.
 
> @@ +355,5 @@
> > +
> > +  mSelfURI = aSelfURI;
> > +
> > +  if (!mSelfURI) {
> > +    aChannel->GetURI(getter_AddRefs(mSelfURI));
> 
> obtain the nsresult and handle any errors.
> 
> @@ +358,5 @@
> > +  if (!mSelfURI) {
> > +    aChannel->GetURI(getter_AddRefs(mSelfURI));
> > +  }
> > +
> > +  NS_ASSERTION(mSelfURI, "No mSelfURI in SetRequestContext");
> 
> Please make the assertion message more actionable.  Why do we need the
> mSelfURI?

Ok, updated.
 
> @@ +438,5 @@
> > +
> > +  // We can't serialize a reference to the principal that triggered this
> > +  // instance to serialize, so when this is deserialized by the principal the
> > +  // caller must hook it up manually by calling setRequestContext on this
> > +  // instance with the appropriate nsIChannel.
> 
> Maybe delete this comment since we don't use the principal anymore.

Deleted.
 
> @@ +440,5 @@
> > +  // instance to serialize, so when this is deserialized by the principal the
> > +  // caller must hook it up manually by calling setRequestContext on this
> > +  // instance with the appropriate nsIChannel.
> > +
> > +  // Finally, serialize all the policies.
> 
> Nit: "Finally" sounds awkward here.

Deleted.




(In reply to Garrett Robinson [:grobinson] from comment #33)
> Comment on attachment 8412261 [details] [diff] [review]
> part_2_contentpolicy_tiein_v2.patch
> 
> Review of attachment 8412261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsCSPContext.cpp
> @@ +69,2 @@
> >    *outDecision = nsIContentPolicy::ACCEPT;
> > +  nsString violatedDirective;
> 
> Nit: local variable should be nsAutoString

Indeed, thanks for pointing this out.
 
> @@ +88,5 @@
> > +  nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryInterface(aRequestContext);
> > +  bool problematicPreload = doc &&
> > +      (aContentType == nsIContentPolicy::TYPE_SCRIPT ||
> > +       aContentType == nsIContentPolicy::TYPE_STYLESHEET);
> > +
> 
> Disagree with Sid's point - this variable indicates that the load is a) a
> preload and b) would be adversely affected if we did not use the special
> logic (because it is script or style, and can be nonced). isPreload is not
> accurate, and cannot be consistently determined (the QI to
> nsIDOMHTMLDocument does not work for all load types).
> 
> @@ +115,5 @@
> > +      }
> > +
> > +      // Do not send a report or notify observers if this is a preload - the
> > +      // decision may be wrong due to the inability to get the nonce, and will
> > +      // incorrectly fail the unit tests.
> 
> Note: the preload phase only blocks things that should be allowed in the
> future. Since we don't have the nonce, we will default to blocking any
> resources that would otherwise be whitelisted by the nonce.
> 
> @@ +162,5 @@
> >  {
> > +#ifdef PR_LOGGING
> > +  if (!gCspContextPRLog) {
> > +    gCspContextPRLog = PR_NewLogModule("CSPContext");
> > +  }
> 
> e.g.
> http://dxr.mozilla.org/mozilla-central/source/security/manager/boot/src/
> nsSiteSecurityService.cpp#38

That is a clean solution for PR_LOGGING. I am going to incorporate that in the parser and utils as well.
 
> @@ +241,5 @@
> >  
> >  NS_IMETHODIMP
> > +nsCSPContext::getAllowsInternal(nsContentPolicyType aContentType,
> > +                                enum CSPKeyword aKeyword,
> > +                                const nsAString& aHashOrNonce,
> 
> Nit: this name is a little confusing. The string here is not actually the
> hash, but the content of the element to be hashed (because we don't know
> what algorithm to use for hashing until we're inside the policy). I used to
> call it aContent but that wasn't great. aHashOrNonce might be ok, although
> maybe you should add a comment to this effect.

Renamed to aNonceOrContent and added comment, how about that?
 
> @@ +279,4 @@
> >  {
> > +  return getAllowsInternal(nsIContentPolicy::TYPE_SCRIPT,
> > +                           CSP_UNSAFE_INLINE,
> > +                           NS_LITERAL_STRING(""),
> 
> Nit: I think EmptyString() is preferred here

EmptyString is the way to go indeed.

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_6_parser_tests_v2.patch

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_0_parser_utils_v4.patch

As discussed on IRC, just unbitrotted the file and made PR_LOGGING consistent with nsCSPContext. Ready to be reviewed!

Comment 41

[Johnny Stenback (:jst)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #35)
[...]
> > +  nsAutoString algo(Substring(expr, 0, dashIndex));
> > +  nsAutoString hash(Substring(expr, dashIndex + 1, expr.Length() -
> > dashIndex + 1));
> > 
> > Replace nsAutoString here with const nsAString&.
> 
> Updated, but nsAString& does not work here:
> 
>  0:12.05 /home/ckerschb/moz/mc/content/base/src/nsCSPParser.cpp:558:14:
> error: non-const lvalue reference to type 'nsAString_internal' cannot bind
> to a temporary of type 'const nsDependentSubstring'
>  0:12.05   nsAString& algo(Substring(expr, 0, dashIndex));

Did you use "nsAString&" instead of "const nsAString&"?

The rest looks good, chances are I'm a bit rusty on PromiseFlatString(), so if something I suggested doesn't compile, chances are my memory failed me.

Comment 42

[Johnny Stenback (:jst)]

Comment on attachment 8418807 [details] [diff] [review]
part_0_parser_utils_v4.patch

This is looking really good! I want to look this over one more time after you've fixed the mEndChar deferencing issues we discussed on irc (and mentioned here too), r- only because of that, and I found a couple of other minor things below as well:

- In nsCSPTokenizer::generateNextToken():

+  skipWhiteSpaceAndSemicolon();
+  while (*mCurChar != WHITESPACE &&
+         *mCurChar != SEMICOLON &&
+         mCurChar < mEndChar) {

Per our discussion on irc you should swap this around so the first check you do is the mCurChar < mEndChar cause you're not supposed to dereference mEndChar, even though it's more than likely safe in this code.

- In nsCSPTokenizer::generateTokens()

+  while (mCurChar < mEndChar) {
+    generateNextToken();
+    dirAndSrcs.AppendElement(mCurToken);
+    skipWhiteSpace();
+    if (accept(SEMICOLON) || mCurChar >= mEndChar) {

Same here, swap the condition order to avoid dereferencing the end of the string. Also probably worth adding assertions in accept() and advance() asserting that we never dereference or step past mEndChar.

+      mTokens.AppendElement(dirAndSrcs);

It looks like this is the only use of mTokens in nsCSPTokenizer. Maybe just pass in the outTokens in nsCSPTokenizer::tokenizeCSPPolicy() to this method instead of storing them as a member on the nsCSPTokenizer class?

- In nsCSPParser::sourceList(nsTArray<nsCSPBaseSrc*>& outSrcs):

+  // remember, srcs start at index 1
+  for (uint32_t i = 1; i < mCurDir.Length(); i++) {
+    // mCurToken is only set here and remains the current token
+    // to be processed, which avoid passing arguments between functions.
+    mCurToken = mCurDir[i];
+
+  CSPPARSERLOG(("nsCSPParser::sourceList, mCurToken: %s, mCurValue: %s",
+               NS_ConvertUTF16toUTF8(mCurToken).get(),
+               NS_ConvertUTF16toUTF8(mCurValue).get()));

Nit: Indent the log stuff two more spaces. Same in reportURIList().

- In nsCSPParser::parseContentSecurityPolicy():

+#ifdef PR_LOGGING
+  CSPPARSERLOG(("nsCSPParser::parseContentSecurityPolicy, policy: %s",
+               NS_ConvertUTF16toUTF8(aPolicyString).get()));
+  nsAutoCString spec;

Please wrap everything inside this #if PR_LOGGING inside {} so that this nsAutoString doesn't leak out to the code that is outside of the #ifdef.

+  nsCSPParser* parser = new nsCSPParser(tokens, aSelfURI, aInnerWindowID);
+
+  // Start the parser to generate a new CSPPolicy using the generated tokens.
+  nsCSPPolicy* policy = parser->policy();
+
+  // Check that report-only policies define a report-uri, otherwise log warning.
+  if (aReportOnly) {
+    policy->setReportOnlyFlag(true);
+    if (!policy->directiveExists(CSP_REPORT_URI)) {
+      nsAutoCString prePath;
+      nsresult rv = aSelfURI->GetPrePath(prePath);
+      NS_ENSURE_SUCCESS(rv, policy);

If rv is ever not a success code this will leak the parser. Given that the parser is deleted further down, does it need to be heap allocated? Seems it could just live on the stack, which would fix this leak as well.

+#ifdef PR_LOGGING
+  nsString parsedPolicy;

Same here, wrap this code in {}. There's a few more of this further down in this patch too, maybe just search for PR_LOGGING and make sure the code inside is scoped to that #ifdef so you don't end up accidentally landing code that doesn't build if PR_LOGGING is *not* defined.

- In content/base/src/nsCSPParser.h, nsCSPTokenizer:


+    inline bool accept(char16_t aChar)
+      { if (*mCurChar == aChar) { mCurToken.Append(*mCurChar++); return true; } else { return false; } }

Please split that on multiple lines for better readability, and remove the else-after-return. And add the asserts we discussed on irc.

+inline const char* CSP_EnumToDirective(enum CSPDirective aDir)
+{
+  // Make sure all elements in enum CSPDirective got added to CSPStrDirectives.
+  NS_ASSERTION((sizeof(CSPStrDirectives) / sizeof(CSPStrDirectives[0]) ==
+               static_cast<uint32_t>(CSP_LAST_DIRECTIVE_VALUE)),
+               "Can not convert unknown Enum to Directive");

I think you should be able to statically assert this (i.e. make the compiler do it instead of relying on the assert firing at runtime). See STATIC_ASSERT etc.

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_0_parser_utils_from_comment_42.patch

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #42)
> Comment on attachment 8418807 [details] [diff] [review]
> part_0_parser_utils_v4.patch
> 
> This is looking really good! I want to look this over one more time after
> you've fixed the mEndChar deferencing issues we discussed on irc (and
> mentioned here too), r- only because of that, and I found a couple of other
> minor things below as well:

Thanks Johnny. I went ahead and made sure that we never derefrence mEndChar putting an atEnd() check in accept(), peek() and advance(), which leaves the rest of the parser basically unchanged, but at the same time makes sure that we never deref mEndChar. Further, I am expanding the testsuite to basically do a simplified fuzzy-test feeding random strings to the parser making sure there is no possibility to crash the parser or also getting stuck in endless loops.

Per our discussion on irc, I exported a separated patch with your latest suggestions incorporated to allow easier reviewing for you.

> - In nsCSPTokenizer::generateNextToken():
> 
> +  skipWhiteSpaceAndSemicolon();
> +  while (*mCurChar != WHITESPACE &&
> +         *mCurChar != SEMICOLON &&
> +         mCurChar < mEndChar) {
> 
> Per our discussion on irc you should swap this around so the first check you
> do is the mCurChar < mEndChar cause you're not supposed to dereference
> mEndChar, even though it's more than likely safe in this code.

Flipped that around, not only in this case but made sure this happens consistently everywhere where we perform such 'end' checks.
 
> - In nsCSPTokenizer::generateTokens()
> 
> +  while (mCurChar < mEndChar) {
> +    generateNextToken();
> +    dirAndSrcs.AppendElement(mCurToken);
> +    skipWhiteSpace();
> +    if (accept(SEMICOLON) || mCurChar >= mEndChar) {
> 
> Same here, swap the condition order to avoid dereferencing the end of the
> string. Also probably worth adding assertions in accept() and advance()
> asserting that we never dereference or step past mEndChar.

As discussed, didn't put assertions but an explicit atEnd() check and returning false in such cases.
 
> +      mTokens.AppendElement(dirAndSrcs);
> 
> It looks like this is the only use of mTokens in nsCSPTokenizer. Maybe just
> pass in the outTokens in nsCSPTokenizer::tokenizeCSPPolicy() to this method
> instead of storing them as a member on the nsCSPTokenizer class?

Yes, simplified that part as well. No need for an additonal member in the class, passing it as an argument.
 
> - In nsCSPParser::sourceList(nsTArray<nsCSPBaseSrc*>& outSrcs):
> 
> +  // remember, srcs start at index 1
> +  for (uint32_t i = 1; i < mCurDir.Length(); i++) {
> +    // mCurToken is only set here and remains the current token
> +    // to be processed, which avoid passing arguments between functions.
> +    mCurToken = mCurDir[i];
> +
> +  CSPPARSERLOG(("nsCSPParser::sourceList, mCurToken: %s, mCurValue: %s",
> +               NS_ConvertUTF16toUTF8(mCurToken).get(),
> +               NS_ConvertUTF16toUTF8(mCurValue).get()));
> 
> Nit: Indent the log stuff two more spaces. Same in reportURIList().
> - In nsCSPParser::parseContentSecurityPolicy():
> 
> +#ifdef PR_LOGGING
> +  CSPPARSERLOG(("nsCSPParser::parseContentSecurityPolicy, policy: %s",
> +               NS_ConvertUTF16toUTF8(aPolicyString).get()));
> +  nsAutoCString spec;
> 
> Please wrap everything inside this #if PR_LOGGING inside {} so that this
> nsAutoString doesn't leak out to the code that is outside of the #ifdef.

Went over all PR_LOGGING and made sure indentation is correct and also wrappend everything inside {} whenever needed in parser and utils.
 
> +  nsCSPParser* parser = new nsCSPParser(tokens, aSelfURI, aInnerWindowID);
> +
> +  // Start the parser to generate a new CSPPolicy using the generated
> tokens.
> +  nsCSPPolicy* policy = parser->policy();
> +
> +  // Check that report-only policies define a report-uri, otherwise log
> warning.
> +  if (aReportOnly) {
> +    policy->setReportOnlyFlag(true);
> +    if (!policy->directiveExists(CSP_REPORT_URI)) {
> +      nsAutoCString prePath;
> +      nsresult rv = aSelfURI->GetPrePath(prePath);
> +      NS_ENSURE_SUCCESS(rv, policy);
> 
> If rv is ever not a success code this will leak the parser. Given that the
> parser is deleted further down, does it need to be heap allocated? Seems it
> could just live on the stack, which would fix this leak as well.

Yep, we already doing this for the tokenizer, don't know why I was not allocating the parser on the stack in the first place.
 
> +#ifdef PR_LOGGING
> +  nsString parsedPolicy;
> Same here, wrap this code in {}. There's a few more of this further down in
> this patch too, maybe just search for PR_LOGGING and make sure the code
> inside is scoped to that #ifdef so you don't end up accidentally landing
> code that doesn't build if PR_LOGGING is *not* defined.

As explained above, fixed that everywhere.
 
> - In content/base/src/nsCSPParser.h, nsCSPTokenizer:
> 
> 
> +    inline bool accept(char16_t aChar)
> +      { if (*mCurChar == aChar) { mCurToken.Append(*mCurChar++); return
> true; } else { return false; } }
> 
> Please split that on multiple lines for better readability, and remove the
> else-after-return. And add the asserts we discussed on irc.

Changed the style of inline functions everywhere. During that cleanup I also removed that unnecessary 'else'.
 
> +inline const char* CSP_EnumToDirective(enum CSPDirective aDir)
> +{
> +  // Make sure all elements in enum CSPDirective got added to
> CSPStrDirectives.
> +  NS_ASSERTION((sizeof(CSPStrDirectives) / sizeof(CSPStrDirectives[0]) ==
> +               static_cast<uint32_t>(CSP_LAST_DIRECTIVE_VALUE)),
> +               "Can not convert unknown Enum to Directive");
> 
> I think you should be able to statically assert this (i.e. make the compiler
> do it instead of relying on the assert firing at runtime). See STATIC_ASSERT
> etc.

STATIC_ASSERT it is in all of those cases.

Comment 44

[Johnny Stenback (:jst)]

Comment on attachment 8419502 [details] [diff] [review]
part_0_parser_utils_from_comment_42.patch

Looks great! r=jst, one minor thing you can consider below.

- In nsCSPTokenizer::generateNextToken():

+  while (mCurChar < mEndChar &&
+         *mCurChar != WHITESPACE &&
+         *mCurChar != SEMICOLON) {

Maybe use the !atEnd() helper here instead of comparing the pointers directyl? Same in a couple of other places, maybe search for mEndChar and change them all?

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_6_parser_tests_v2.patch

Talking with jst about the parser and possible random input we might get from users encouraged us to extend the parser testsuite to also contain fuzzy tests. We randomly (tough deterministically) generate random policies and feed them as input to the parser. Due to this change the old patch currently marked for review is outdated.

Comment 46

[Sid Stamm [:geekboy or :sstamm]]

part_0_parser_utils_v4.patch (68.61 KB, patch)
part_0_parser_utils_from_comment_42.patch (23.55 KB, patch)

Is one of these obsolete or do we need both applied to compile and run?

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_0_parser_utils_v5.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #46)
> part_0_parser_utils_v4.patch (68.61 KB, patch)
> part_0_parser_utils_from_comment_42.patch (23.55 KB, patch)
> 
> Is one of these obsolete or do we need both applied to compile and run?

In fact, both of them are. Haven't uploaded the combined patch yet (including latest nitpick changes).

Here we go!

Comment 48

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8420313 [details] [diff] [review]
part_6_parser_tests_v2.patch

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

Is there any sort of debugging we can do?  PR logging or equivalent?  For when this fails, it would be nice to have some good output.

The fuzzy tests might be overkill to run with each commit, but good to have them available.  We discussed this in person, we could get better coverage with less time commitment per patch if we run a random subset of the possible fuzzy tests each time.  So long as there is additional logging that spits out failed test cases (NS_ASSERTION, etc) this would be sufficient.

r=me if you address these comments and:
1. Make sure the time taken to run these on tinderbox is not obscene with the fuzz tests (no timeouts, not super-long)
2. jst likes the approach for the fuzzing (flagging jst for review of those parts)
3. you either add more detailed output on failures (for easier debugging) and re-flag for review or file a followup bug to add more debugging output to these tests.

::: content/base/test/TestCSPParser.cpp
@@ +43,5 @@
> +//      using the struct PolicyTest;
> +//
> +// You can use TestSimplePolicies() as a template for writing your tests.
> +
> +static const uint32_t kMaxPolicyLength = 96;

Will we ever want to test policies longer than 96 chars?

@@ +47,5 @@
> +static const uint32_t kMaxPolicyLength = 96;
> +// For fuzzy testing we actually do not care about the output,
> +// we just want to make sure that the parser can handle random
> +// input, therefore we use kFuzzyExpectedPolicyCount to return early.
> +static const uint32_t kFuzzyExpectedPolicyCount = 111;

Please explain what "fuzzy testing" is for readers of this file.

@@ +69,5 @@
> +    do_CreateInstance(NS_CSPCONTEXT_CONTRACTID, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  csp->SetRequestContext(selfURI, nullptr, nullptr, nullptr);
> +  NS_ENSURE_SUCCESS(rv, rv);

Please put a comment here about why all the nullptr args are ok for testing purposes

@@ +79,5 @@
> +  // because we are using the selfURI set in SetRequestingContext
> +  rv = csp->AppendPolicy(policyStr, nullptr, false, true);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // exit early when executing fuzzy tests

please explain why.  ("since the result doesn't matter, we are just testing for crashes")

@@ +88,5 @@
> +  // verify that the expected number of policies exists
> +  uint32_t actualPolicyCount;
> +  rv = csp->GetPolicyCount(&actualPolicyCount);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(actualPolicyCount == aExpectedPolicyCount, NS_ERROR_FAILURE);

NS_ENSURE_STATE(actualPolicyCount == aExpectedPolicyCount) returns NS_ERROR_UNEXPECTED, which makes sense here.

@@ +102,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_ENSURE_TRUE(
> +    NS_ConvertUTF16toUTF8(parsedPolicyStr).EqualsASCII(aExpextedResult),
> +    NS_ERROR_FAILURE);

Would you like this test to crash with an assertion message if the test fails (instead of just quietly failing)?  If so, NS_POSTCONDITION would work here.  If you go this route, it might make sense to sprinkle NS_POSTCONDITION and NS_ASSERTION calls throughout these tests to break on failure with a string.

@@ +753,5 @@
> +    // randomly select the length of the next policy
> +    uint32_t polLength = rand() % (kMaxPolicyLength - defaultSrcLen);
> +    // reset memory of the policy string, but leave default-src.
> +    memset((&(testPol[0].policy) + (defaultSrcLen * sizeof(char))),
> +           '\0', (kMaxPolicyLength - defaultSrcLen) * sizeof(char));

Is there a reason you're using memset and malloc instead of our string libraries (and .Assign(), etc)?

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8420313 [details] [diff] [review]
part_6_parser_tests_v2.patch

As discussed in person, we will have two different modes for testing the CSPParser. The compiled code test suite hides the fuzzy tests behind a switch, which can be flipped to test the parser using random input *OFFLINE*. Using fuzzy inputs provides additional coverage but doesn't need to be executed every time we run tests on TBPL.

Cancelling your (jst's) review since you said you are fine with sid's r+.

Comment 50

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_0_parser_utils_ready.patch

Going to export all patches again to make sure no minor changes got left behind.

Comment 51

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_1_backend_stubs_ready.patch

Going to export all patches again to make sure no minor changes got left behind.

Comment 52

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_2_contentpolicy_tiein_ready.patch

Going to export all patches again to make sure no minor changes got left behind.

Comment 53

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_3_parser_tests_ready.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #48)
> Comment on attachment 8420313 [details] [diff] [review]
> part_6_parser_tests_v2.patch
> 
> Review of attachment 8420313 [details] [diff] [review]:
> -----------------------------------------------------------------

After discussing our testing strategy with sid and jst we decided to have two modes for testing the parser. An offline and an online mode. The offline mode extends parser tests by also performing fuzzy tests which we can run whenever we update the parser, but there is no need that fuzzy testing is run everytime we push to try.

> Is there any sort of debugging we can do?  PR logging or equivalent?  For
> when this fails, it would be nice to have some good output.
Yes, other compiled code tests use fail() which I added to the testsuite.

> The fuzzy tests might be overkill to run with each commit, but good to have
> them available.  We discussed this in person, we could get better coverage
> with less time commitment per patch if we run a random subset of the
> possible fuzzy tests each time.  So long as there is additional logging that
> spits out failed test cases (NS_ASSERTION, etc) this would be sufficient.
> 
> r=me if you address these comments and:
> 1. Make sure the time taken to run these on tinderbox is not obscene with
> the fuzz tests (no timeouts, not super-long)
As discussed above, since we already have the fuzzy tests, we can use the offline whenever we perform changes in the parser.
> 2. jst likes the approach for the fuzzing (flagging jst for review of those
> parts)
He is fine with your r+.

> 3. you either add more detailed output on failures (for easier debugging)
> and re-flag for review or file a followup bug to add more debugging output
> to these tests.
Added more debugging output.
 
> ::: content/base/test/TestCSPParser.cpp
> @@ +43,5 @@
> > +//      using the struct PolicyTest;
> > +//
> > +// You can use TestSimplePolicies() as a template for writing your tests.
> > +
> > +static const uint32_t kMaxPolicyLength = 96;
> 
> Will we ever want to test policies longer than 96 chars?

As of now, I think 96 should be sufficient, if we ever want to change that, we just have to change that static variable.
 
> @@ +47,5 @@
> > +static const uint32_t kMaxPolicyLength = 96;
> > +// For fuzzy testing we actually do not care about the output,
> > +// we just want to make sure that the parser can handle random
> > +// input, therefore we use kFuzzyExpectedPolicyCount to return early.
> > +static const uint32_t kFuzzyExpectedPolicyCount = 111;
> 
> Please explain what "fuzzy testing" is for readers of this file.
> 
> @@ +69,5 @@
> > +    do_CreateInstance(NS_CSPCONTEXT_CONTRACTID, &rv);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  csp->SetRequestContext(selfURI, nullptr, nullptr, nullptr);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> Please put a comment here about why all the nullptr args are ok for testing
> purposes

Added comment.

 
> @@ +79,5 @@
> > +  // because we are using the selfURI set in SetRequestingContext
> > +  rv = csp->AppendPolicy(policyStr, nullptr, false, true);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  // exit early when executing fuzzy tests
> 
> please explain why.  ("since the result doesn't matter, we are just testing
> for crashes")

Explained more detailed.

> @@ +88,5 @@
> > +  // verify that the expected number of policies exists
> > +  uint32_t actualPolicyCount;
> > +  rv = csp->GetPolicyCount(&actualPolicyCount);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  NS_ENSURE_TRUE(actualPolicyCount == aExpectedPolicyCount, NS_ERROR_FAILURE);
> 
> NS_ENSURE_STATE(actualPolicyCount == aExpectedPolicyCount) returns
> NS_ERROR_UNEXPECTED, which makes sense here.

Returning NS_ERROR_UNEXPECTED in all error cases.
 
> @@ +102,5 @@
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  NS_ENSURE_TRUE(
> > +    NS_ConvertUTF16toUTF8(parsedPolicyStr).EqualsASCII(aExpextedResult),
> > +    NS_ERROR_FAILURE);
> 
> Would you like this test to crash with an assertion message if the test
> fails (instead of just quietly failing)?  If so, NS_POSTCONDITION would work
> here.  If you go this route, it might make sense to sprinkle
> NS_POSTCONDITION and NS_ASSERTION calls throughout these tests to break on
> failure with a string.

Updated that part and added debug output using |fail()|.

> @@ +753,5 @@
> > +    // randomly select the length of the next policy
> > +    uint32_t polLength = rand() % (kMaxPolicyLength - defaultSrcLen);
> > +    // reset memory of the policy string, but leave default-src.
> > +    memset((&(testPol[0].policy) + (defaultSrcLen * sizeof(char))),
> > +           '\0', (kMaxPolicyLength - defaultSrcLen) * sizeof(char));
> 
> Is there a reason you're using memset and malloc instead of our string
> libraries (and .Assign(), etc)?

No particular reason, the tests have grown over time, where we started with const chars* back then. I think it's not worth the effort to update that at this point.

Comment 54

[Christoph Kerschbaumer [:ckerschb]]

I think this is ready to go, pushing to try to make sure everything is peachy:
https://tbpl.mozilla.org/?tree=Try&rev=0615473a02dc

Please remember that this is going to land and live behind |security.csp.newbackend.enable| for some time.

Comment 55

[Garrett Robinson [:grobinson]]

Try looks good, except for the failures of b2g/components/test/mochitest/test_permission_deny.html. ckerschb, can you take a look at those?

Comment 56

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Garrett Robinson [:grobinson] from comment #55)
> Try looks good, except for the failures of
> b2g/components/test/mochitest/test_permission_deny.html. ckerschb, can you
> take a look at those?

To me that seems like an intermittent failure, I already restarted that test a while ago, it's green now.

The other thing is, even though this patch is not causing the problem for the Emulator, why are those 'burning'? I restarted them one after the other.

Anyway, I think this code is ready to land, we are definitely not causing the problem here.

Comment 57

[Garrett Robinson [:grobinson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73a818b6179f
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc6c3b1d8aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4ec5597daa
https://hg.mozilla.org/integration/mozilla-inbound/rev/57a9d6af1a1e

Comment 58

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/73a818b6179f
https://hg.mozilla.org/mozilla-central/rev/dbc6c3b1d8aa
https://hg.mozilla.org/mozilla-central/rev/5d4ec5597daa
https://hg.mozilla.org/mozilla-central/rev/57a9d6af1a1e

Comment 59

[Christoph Kerschbaumer [:ckerschb]]

Benjamin, can you add me to 1018829, can't view it. Thanks!