790747 - Move InitCSP Code to before reset call in StartDocumentLoad

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Devdatta Akhawe [:devd]]

This is a tracking bug to ease pushing the r+'ed "move csp code to before principal reset" patch from Bug 671389.

Comment 1

[Devdatta Akhawe [:devd]]

Attachment: Bug 671389 Move CSP code to before principal reset

carrying over the r+ from bug 671389

Comment 2

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

Comment 3

[Devdatta Akhawe [:devd]]

Comment on attachment 660579 [details] [diff] [review]
Move CSP code to before principal reset

carrying over r+ from bz

Comment 4

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

Comment 5

[Devdatta Akhawe [:devd]]

Comment on attachment 660870 [details] [diff] [review]
Move CSP code to before principal reset

Changed 2 vars from nsCAutoString to nsAutoCString; for some reason, it failed to compile with nsCAutoString. Carrying over r+

Comment 6

[Devdatta Akhawe [:devd]]

Try run looks good to me. https://tbpl.mozilla.org/?tree=Try&rev=e4a99794b88a

I only did windows, since I don't think there are any OS specific components here.

@imelven: Can you push this?

Comment 7

[Ian Melven :imelven]

Comment on attachment 660870 [details] [diff] [review]
Move CSP code to before principal reset

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

Not to be 'that developer', but there's quite a few style/spacing/whitespace nits still left in this patch.

::: content/base/src/nsDocument.cpp
@@ +2425,5 @@
>      nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags);
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
> +  if(csp){

spacing is a little off

@@ +2430,5 @@
> +    // Copy into principal
> +    nsIPrincipal* principal = GetPrincipal();
> +    principal->SetCsp(csp);
> +#ifdef PR_LOGGING
> +    PR_LOG(gCspPRLog, PR_LOG_DEBUG, 

trailing whitespace

@@ +2431,5 @@
> +    nsIPrincipal* principal = GetPrincipal();
> +    principal->SetCsp(csp);
> +#ifdef PR_LOGGING
> +    PR_LOG(gCspPRLog, PR_LOG_DEBUG, 
> +        ("Inserted CSP into principal %p", principal));

inconsistent indentation ?

@@ +2446,2 @@
>    if (CSPService::sCSPEnabled) {
> +    nsAutoCString tCspHeaderValue,tCspROHeaderValue;

space after comma

@@ +2455,5 @@
> +        tCspHeaderValue);
> +
> +    httpChannel->GetResponseHeader(
> +        NS_LITERAL_CSTRING("x-content-security-policy-report-only"),
> +        tCspROHeaderValue);

i'm not sure of the correct style here and couldn't find any examples.. maybe 2 spaces and not 4 though

@@ +2553,3 @@
>          }
>      }
> +    

whitespace

Comment 8

[Ian Melven :imelven]

I agree that it's probably fine to only do Windows on try for this too, fwiw.

Comment 9

[Devdatta Akhawe [:devd]]

fixed them all, save for 
> 
> @@ +2431,5 @@
> > +    nsIPrincipal* principal = GetPrincipal();
> > +    principal->SetCsp(csp);
> > +#ifdef PR_LOGGING
> > +    PR_LOG(gCspPRLog, PR_LOG_DEBUG, 
> > +        ("Inserted CSP into principal %p", principal));
> 
> inconsistent indentation ?
>

I don't know the rules here, what is the correct indentation?

> i'm not sure of the correct style here and couldn't find any examples..
> maybe 2 spaces and not 4 though
>

4 seems right to me. See line 287 in the current code for example.
 
> @@ +2553,3 @@
> >          }
> >      }
> > +    
> 
> whitespace

I can't find this. A search for 2553 in the attachment didn't bring anything up. Can you give more context for this one?

Comment 10

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

Comment 11

[Ian Melven :imelven]

(In reply to Devdatta Akhawe from comment #9)

Thank you :)

> > @@ +2431,5 @@
> > > +    nsIPrincipal* principal = GetPrincipal();
> > > +    principal->SetCsp(csp);
> > > +#ifdef PR_LOGGING
> > > +    PR_LOG(gCspPRLog, PR_LOG_DEBUG, 
> > > +        ("Inserted CSP into principal %p", principal));
> > 
> > inconsistent indentation ?
> >
> 
> I don't know the rules here, what is the correct indentation?
> 

See, for example, http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#1568

> > i'm not sure of the correct style here and couldn't find any examples..
> > maybe 2 spaces and not 4 though
> >
> 
> 4 seems right to me. See line 287 in the current code for example.

AFAIK only nsDocshell.cpp uses 4 spaces, but I'm not going to be difficult about it :) 

> > @@ +2553,3 @@
> > >          }
> > >      }
> > > +    
> > 
> > whitespace
> 
> I can't find this. A search for 2553 in the attachment didn't bring anything
> up. Can you give more context for this one?

In splinter, it looks like there's some leading whitespace on the line before csp.forget(aCSP); in nsDocument.cpp, see https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=790747&attachment=660870 - sorry, I should have given you more context than splinter did there !

Thanks again for cleaning this up !

Comment 12

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

Comment 13

[Devdatta Akhawe [:devd]]

aah. The whitespace was intentional (I thought that was the spacing requirement). Anyways, that's fixed, as is the other issue. I am keeping the 4 spaces.

Comment 14

[Ian Melven :imelven]

(In reply to Devdatta Akhawe from comment #13)
> aah. The whitespace was intentional (I thought that was the spacing
> requirement). Anyways, that's fixed, as is the other issue. I am keeping the
> 4 spaces.

It's not the blank line that's the problem, it was some whitespace on that blank line - I know, very nitpicky.. I'll land this tomorrow if someone else doesn't pick it up from the checkin-needed. Thanks again !

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

so, the patch that's on this bug and not obsolete has no review on it.  Has it been reviewed?  Or if one of the previous reviews should carry over, can you mark it and mention who reviewed it in the bug so it can be marked when checked in?

Comment 16

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #15)
> so, the patch that's on this bug and not obsolete has no review on it.  Has
> it been reviewed?  Or if one of the previous reviews should carry over, can
> you mark it and mention who reviewed it in the bug so it can be marked when
> checked in?

bz reviewed this and the only changes since then have been style/whitespace nits, so it seems fine to carry over the r+ imo - see comment 3

Comment 17

[Ian Melven :imelven]

See https://bugzilla.mozilla.org/show_bug.cgi?id=671389#c58 for Boris' review of this patch

Comment 18

[Ian Melven :imelven]

Comment on attachment 661110 [details] [diff] [review]
Move CSP code to before principal reset

Carrying over the r+ as described above.

Comment 19

[Ian Melven :imelven]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca55c9bdfd4e

Comment 20

[Ian Melven :imelven]

Latest try run is in comment 6, there are some oranges on the push.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca55c9bdfd4e