Closed
Bug 1349517
Opened 6 years ago
Closed 6 years ago
Assertion failure: !mCSP (do not destroy an existing CSP), at nsNullPrincipal.cpp:116
Categories
(Core :: DOM: Security, defect, P2)
Core
DOM: Security
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)
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•6 years ago
|
||
I'll take this for investigation.
Assignee: nobody → fbraun
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
The website stopped showing this behavior. Can't reproduce anymore.
Assignee | ||
Comment 3•6 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•6 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•6 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•6 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)
Updated•6 years ago
|
Priority: -- → P2
Whiteboard: [domsecurity-active]
Assignee | ||
Comment 8•6 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•6 years ago
|
||
Tested about 1½hrs in total, no more assertion failures.
Comment 11•6 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•6 years ago
|
||
I have triggered a try push. Please check-in, once it has succeeded.
Keywords: checkin-needed
Comment 14•6 years ago
|
||
Autoland can't push this until all pending issues are marked as resolved in MozReview.
Keywords: checkin-needed
Assignee | ||
Comment 15•6 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•6 years ago
|
||
Ah, I'm still getting the hang of MozReview. Please try to check in again
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 17•6 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5080c78d2515
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
status-firefox52:
--- → unaffected
status-firefox53:
--- → unaffected
status-firefox54:
--- → unaffected
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•