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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Eric Rahm [:erahm]]

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Security checks within nsContentSecurityManager return NS_ERROR_* whenever a security check fails. Is that a problem?

Comment 2

[Eric Rahm [: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

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 5

[Tanvi Vyas[:tanvi]]

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.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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+.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aff31c387150

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aff31c387150