27,800 instances of "NS_ENSURE_SUCCESS(rv, NS_OK) failed with result 0x80004005" emitted from dom/security/nsContentSecurityManager.cpp during linux64 debug testing

RESOLVED FIXED in Firefox 45

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: erahm, Assigned: ckerschb)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

In the past data URLs that are used in testing have tripped up warnings like this. We could possibly just add that to previous scheme check and move on.

> 27762 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, NS_OK) failed with result 0x80004005: file dom/security/nsContentSecurityManager.cpp, line 436

This warning [1], introduced in bug 1221365, shows up in the following test suites:

> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-8-bm123-tests1-linux64-build9.txt:1913
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-8-bm116-tests1-linux64-build15.txt:1676
> mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm68-tests1-linux64-build4.txt:1152
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-7-bm54-tests1-linux64-build12.txt:883
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-7-bm122-tests1-linux64-build1.txt:874
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-6-bm125-tests1-linux64-build15.txt:859
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-6-bm53-tests1-linux64-build7.txt:859
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-reftests-e10s-bm118-tests1-linux64-build11.txt:802
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-3-bm116-tests1-linux64-build23.txt:799
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-1-bm125-tests1-linux64-build12.txt:777
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-1-bm118-tests1-linux64-build37.txt:776
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-1-bm125-tests1-linux64-build15.txt:746
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-3-bm113-tests1-linux64-build7.txt:739
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-3-bm53-tests1-linux64-build34.txt:739
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-6-bm53-tests1-linux64-build24.txt:731
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-2-bm67-tests1-linux64-build10.txt:697
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-2-bm68-tests1-linux64-build41.txt:697
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-2-bm122-tests1-linux64-build9.txt:684
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-7-bm118-tests1-linux64-build7.txt:666
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-7-bm118-tests1-linux64-build28.txt:666
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm122-tests1-linux64-build43.txt:637
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-4-bm115-tests1-linux64-build15.txt:632
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-4-bm118-tests1-linux64-build1.txt:629
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-5-bm54-tests1-linux64-build9.txt:590
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-4-bm54-tests1-linux64-build32.txt:576
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-5-bm116-tests1-linux64-build2.txt:575
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-1-bm68-tests1-linux64-build18.txt:501
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-6-bm68-tests1-linux64-build8.txt:473
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-2-bm68-tests1-linux64-build6.txt:420
> mozilla-central_ubuntu64_vm-debug_test-mochitest-jetpack-bm53-tests1-linux64-build25.txt:405
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-reftests-bm68-tests1-linux64-build6.txt:403
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-6-bm122-tests1-linux64-build8.txt:383
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm122-tests1-linux64-build13.txt:376
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-3-bm122-tests1-linux64-build56.txt:360
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-8-bm54-tests1-linux64-build4.txt:335
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm123-tests1-linux64-build11.txt:326
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-7-bm118-tests1-linux64-build1.txt:268
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm68-tests1-linux64-build7.txt:248
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm125-tests1-linux64-build3.txt:242
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-5-bm116-tests1-linux64-build11.txt:237
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-5-bm125-tests1-linux64-build9.txt:236
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-4-bm122-tests1-linux64-build7.txt:188
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-5-bm118-tests1-linux64-build4.txt:159
> mozilla-central_ubuntu64_vm-debug_test-mochitest-1-bm52-tests1-linux64-build12.txt:155
> mozilla-central_ubuntu64_vm-debug_test-mochitest-3-bm123-tests1-linux64-build1.txt:124
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-3-bm116-tests1-linux64-build7.txt:102
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-2-bm123-tests1-linux64-build19.txt:101
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-5-bm122-tests1-linux64-build45.txt:95
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-4-bm118-tests1-linux64-build51.txt:89
> mozilla-central_ubuntu64_vm-debug_test-mochitest-5-bm116-tests1-linux64-build1.txt:72
> mozilla-central_ubuntu64_vm-debug_test-mochitest-2-bm115-tests1-linux64-build3.txt:62
> mozilla-central_ubuntu64_vm-debug_test-mochitest-4-bm115-tests1-linux64-build72.txt:34
> mozilla-central_ubuntu64_vm-debug_test-crashtest-e10s-bm115-tests1-linux64-build3.txt:2
> mozilla-central_ubuntu64_vm-debug_test-crashtest-bm118-tests1-linux64-build33.txt:1

It shows up in     9337 tests. A few of the most prevalent:

> 2064 - /_mozilla/service-workers/service-worker/fetch-event-redirect.https.html
> 578 - Shutdown
> 118 - toolkit/mozapps/extensions/test/browser/browser_discovery.js
> 114 - browser/components/preferences/in-content/tests/browser_cookies_exceptions.js
> 111 - browser/base/content/test/general/browser_bug561636.js
> 84 - browser/components/sessionstore/test/browser_615394-SSWindowState_events.js
> 80 - toolkit/mozapps/extensions/test/browser/browser_bug562797.js
> 80 - jetpack-package/addon-sdk/source/test/test-simple-prefs.js.testUnloadOfDynamicPrefGeneration
> 78 - toolkit/mozapps/extensions/test/browser/browser_types.js
> 71 - browser/components/sessionstore/test/browser_354894_perwindowpb.js

[1] https://hg.mozilla.org/mozilla-central/annotate/a523d4c7efe2/dom/security/nsContentSecurityManager.cpp#l436
(Assignee)

Comment 1

2 years ago
Security checks within nsContentSecurityManager return NS_ERROR_* whenever a security check fails. Is that a problem?
Flags: needinfo?(erahm)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Security checks within nsContentSecurityManager return NS_ERROR_* whenever a
> security check fails. Is that a problem?

The problem is NS_ENSURE_SUCCESS emits a warning on debug builds [1], during testing this happens 28,000 times in |nsContentSecurityManager::IsURIPotentiallyTrustworthy| [2] even though we're just returning NS_OK anyhow.

My understanding is we no longer recommend using NS_ENSURE_*, and rather an explicit |if(NS_FAILED(...))| for new code.

[1] https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/xpcom/glue/nsDebug.h#293,300-307
[2] https://hg.mozilla.org/mozilla-central/annotate/a523d4c7efe2/dom/security/nsContentSecurityManager.cpp#l436
Flags: needinfo?(erahm)
(Assignee)

Comment 3

2 years ago
(In reply to Eric Rahm [:erahm] from comment #2)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> > Security checks within nsContentSecurityManager return NS_ERROR_* whenever a
> > security check fails. Is that a problem?
> 
> The problem is NS_ENSURE_SUCCESS emits a warning on debug builds [1], during
> testing this happens 28,000 times in
> |nsContentSecurityManager::IsURIPotentiallyTrustworthy| [2] even though
> we're just returning NS_OK anyhow.
> 
> My understanding is we no longer recommend using NS_ENSURE_*, and rather an
> explicit |if(NS_FAILED(...))| for new code.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/xpcom/glue/nsDebug.h#293,300-307
> [2]
> https://hg.mozilla.org/mozilla-central/annotate/a523d4c7efe2/dom/security/
> nsContentSecurityManager.cpp#l436

Ah, now I see what's going on, that's within IsURIPotentiallyTrustworthy. I'll get that fixed.
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Created attachment 8689727 [details] [diff] [review]
bug_1226324_update_uri_istrustworthy.patch

We could either use if (NS_FAILED(rv)) { return NS_OK; } or we just let it go and don't do any of those checks, because we would just perform string comparisons on the empty string which is the better option in my opinion.
Attachment #8689727 - Flags: review?(tanvi)

Comment 5

2 years ago
Comment on attachment 8689727 [details] [diff] [review]
bug_1226324_update_uri_istrustworthy.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Created attachment 8689727 [details] [diff] [review]
> bug_1226324_update_uri_istrustworthy.patch
> 
> We could either use if (NS_FAILED(rv)) { return NS_OK; } or we just let it
> go and don't do any of those checks, because we would just perform string
> comparisons on the empty string which is the better option in my opinion.

I lean towards adding the NS_FAILED's so that we avoid the unnecessary string comparisons for at least the host-less URIs.
Attachment #8689727 - Flags: review?(tanvi) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8689747 [details] [diff] [review]
bug_1226324_update_uri_istrustworthy.patch

NS_FAILED(rv) it is. Probably worth saving a few cycles not performing unnecessary string comparisons. Carrying over r+.
Attachment #8689727 - Attachment is obsolete: true
Attachment #8689747 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/aff31c387150
Keywords: checkin-needed

Comment 8

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