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

RESOLVED FIXED in Firefox 55

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cbook, Assigned: freddyb)

Tracking

(Blocks 1 bug, {assertion})

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [domsecurity-active], URL)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Posted 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 ?
(Assignee)

Comment 1

2 years ago
I'll take this for investigation.
Assignee: nobody → fbraun
Status: NEW → ASSIGNED
See Also: → 965637
(Assignee)

Comment 2

2 years ago
The website stopped showing this behavior. Can't reproduce anymore.
(Assignee)

Comment 3

2 years ago
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
(Reporter)

Comment 4

2 years ago
it seems this might also be ad related, could crash once but not on a re-try
Comment hidden (mozreview-request)

Comment 6

2 years ago
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

2 years ago
mozreview-review
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]
(Assignee)

Comment 8

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 10

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

Comment 11

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
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
(Assignee)

Comment 15

2 years ago
mozreview-review
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+
(Assignee)

Comment 16

2 years ago
Ah, I'm still getting the hang of MozReview.
Please try to check in again
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 17

2 years ago
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
(Reporter)

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5080c78d2515
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.