1349517 - Assertion failure: !mCSP (do not destroy an existing CSP), at nsNullPrincipal.cpp:116

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

Assertion failure: !mCSP (do not destroy an existing CSP), at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/caps/nsNullPrincipal.cpp:116

found via bughunter and reproduced on latest firefox debug build on trunk with windows 7. 

STR:

-> Load https://huawei-hisuite.softonic.com/descargar#downloading
--> in testing this caused the assertion failure after reloading that page 1-2 times
---> Assertion failure

Assertion failure: !mCSP (do not destroy an existing CSP), at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/caps/nsNullPrincipal.cpp:116

regression of bug 1073952 ?

Comment 1

[Frederik Braun [:freddy] (out until Feb 6th)]

I'll take this for investigation.

Comment 2

[Frederik Braun [:freddy] (out until Feb 6th)]

The website stopped showing this behavior. Can't reproduce anymore.

Comment 3

[Frederik Braun [:freddy] (out until Feb 6th)]

Tomcat managed to find some other web pages that cause the assertion failure. Sweet.
https://www.last.fm/user/Nimbie/library/artists?page=9&date_preset=ALL and
https://einthusan.tv/movie/watch/6RFT/?lang=hindi where other urls

Comment 4

[Carsten Book [:Tomcat]]

it seems this might also be ad related, could crash once but not on a re-try

Comment 5

[Frederik Braun [:freddy] (out until Feb 6th)]

Attachment: Bug 1349517: don't set CSP on NullPrincipal if it already has one

Review commit: https://reviewboard.mozilla.org/r/123108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123108/

Comment 6

[Bob Clary [:bc] (inactive)]

Another is https://chrome.softonic.com/ which is also intermittent. I was able to get it to "reproduce" using the developer tools web console.

1. load https://chrome.softonic.com/
2. open developer tools/web console
3. allow popups from chrome.softonic.com
4. window.open('about:blank')
5. go to the new tab and load developer tools/web console in the blank window.
6. setInterval('opener.document.location.reload()', 60000)
7. Have some coffee and come back later.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8850504 [details]
Bug 1349517: don't set CSP on NullPrincipal if it already has one

https://reviewboard.mozilla.org/r/123108/#review126704

Does that patch (also when incorporating my suggestion) work for the STRs provided in comment 3 and 6?

::: caps/nsScriptSecurityManager.cpp:323
(Diff revision 1)
> +                  MOZ_ASSERT(nullPrincipalCSP != originalCsp,
> +                             "There should be no other CSP here.");
> +                  // CSPs are equal, no need to set it again.
> +                  return NS_OK;
> +                }
> +                if (!nullPrincipalCSP && originalCsp) {

I think we should slightly modify that code. Mostly because we shouldn't perform any actions if there is no originalCSP. I suggest we do the following.

if (originalCSP) {
  query nullPrincipalCSP;
  if (nullPrincipalCSP) {
    MOZ_ASSERT(originalCSP == nullPrincipalCSP, "they need to be the same right?";
    // if there is a CSP then they need to be the same, otherwise where is the different CSP coming from?
  }
  else {
    principal->setOriginalCSP(originalCSP);
  }
}

Comment 8

[Frederik Braun [:freddy] (out until Feb 6th)]

New round coming up, addressing your review comments.
I let it run for about 45 minutes against the test from comment 6 and comment 3 and will continue for a little longer.

Comment 9

[Frederik Braun [:freddy] (out until Feb 6th)]

Comment on attachment 8850504 [details]
Bug 1349517: don't set CSP on NullPrincipal if it already has one

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123108/diff/1-2/

Comment 10

[Frederik Braun [:freddy] (out until Feb 6th)]

Tested about 1½hrs in total, no more assertion failures.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8850504 [details]
Bug 1349517: don't set CSP on NullPrincipal if it already has one

https://reviewboard.mozilla.org/r/123108/#review127574

::: caps/nsScriptSecurityManager.cpp:322
(Diff revision 2)
> +                  (*aPrincipal)->GetCsp(getter_AddRefs(nullPrincipalCSP));
> +                  if (nullPrincipalCSP) {
> +                  MOZ_ASSERT(nullPrincipalCSP == originalCSP,
> +                              "There should be no other CSP here.");
> +                  // CSPs are equal, no need to set it again.
> +                  return NS_OK;

nit: please use two space indendation within the if-clause.

Comment 12

[Frederik Braun [:freddy] (out until Feb 6th)]

Comment on attachment 8850504 [details]
Bug 1349517: don't set CSP on NullPrincipal if it already has one

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123108/diff/2-3/

Comment 13

[Frederik Braun [:freddy] (out until Feb 6th)]

I have triggered a try push. Please check-in, once it has succeeded.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues are marked as resolved in MozReview.

Comment 15

[Frederik Braun [:freddy] (out until Feb 6th)]

Comment on attachment 8850504 [details]
Bug 1349517: don't set CSP on NullPrincipal if it already has one

https://reviewboard.mozilla.org/r/123108/#review127926

carrying over r+

Comment 16

[Frederik Braun [:freddy] (out until Feb 6th)]

Ah, I'm still getting the hang of MozReview.
Please try to check in again

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5080c78d2515
Don't set CSP on NullPrincipal if it already has one. r=ckerschb

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5080c78d2515