Closed Bug 1349517 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: cbook, Assigned: freddy)

References

()

Details

(Keywords: assertion, Whiteboard: [domsecurity-active])

Attachments

(2 files)

Attached file 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 ?
I'll take this for investigation.
Assignee: nobody → fbraun
Status: NEW → ASSIGNED
See Also: → 965637
The website stopped showing this behavior. Can't reproduce anymore.
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
it seems this might also be ad related, could crash once but not on a re-try
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 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);
  }
}
Attachment #8850504 - Flags: review?(ckerschb)
Priority: -- → P2
Whiteboard: [domsecurity-active]
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.
Tested about 1½hrs in total, no more assertion failures.
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.
Attachment #8850504 - Flags: review?(ckerschb) → review+
I have triggered a try push. Please check-in, once it has succeeded.
Keywords: checkin-needed
Autoland can't push this until all pending issues are marked as resolved in MozReview.
Keywords: checkin-needed
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+
Attachment #8850504 - Flags: review+
Ah, I'm still getting the hang of MozReview.
Please try to check in again
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5080c78d2515
Don't set CSP on NullPrincipal if it already has one. r=ckerschb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5080c78d2515
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.