1262948 - 3,500 instances of "'NS_FAILED(rv)'" emitted from dom/base/URL.cpp during linux64 debug testing

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Eric Rahm [:erahm]]

It looks like this warning was introduced in bug 1160628, it seems to have regressed some time between 12/04/2015 and 03/01/2016.

> 3491 [NNNNN] WARNING: 'NS_FAILED(rv)', file dom/base/URL.cpp, line 96

This warning [1] shows up in the following test suites:

> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-8-bm122-tests1-linux64-build1.txt:791
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm131-tests1-linux64-build5.txt:563
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-5-bm131-tests1-linux64-build5.txt:402
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-4-bm115-tests1-linux64-build2.txt:402
> mozilla-central_ubuntu64_vm-debug_test-mochitest-8-bm54-tests1-linux64-build0.txt:314
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-8-bm131-tests1-linux64-build0.txt:260
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-8-bm118-tests1-linux64-build3.txt:260
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-7-bm113-tests1-linux64-build4.txt:62
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-7-bm116-tests1-linux64-build0.txt:62
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-e10s-3-bm121-tests1-linux64-build5.txt:61
> mozilla-central_ubuntu64_vm-debug_test-web-platform-tests-3-bm116-tests1-linux64-build8.txt:61
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-6-bm54-tests1-linux64-build4.txt:59
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm53-tests1-linux64-build5.txt:53
> mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm131-tests1-linux64-build4.txt:51
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-7-bm68-tests1-linux64-build6.txt:26
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-3-bm118-tests1-linux64-build2.txt:12
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm117-tests1-linux64-build8.txt:10
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-5-bm68-tests1-linux64-build4.txt:4
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-6-bm118-tests1-linux64-build4.txt:4
> mozilla-central_ubuntu64_vm-debug_test-mochitest-5-bm122-tests1-linux64-build8.txt:4
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-6-bm53-tests1-linux64-build5.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-2-bm131-tests1-linux64-build4.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm131-tests1-linux64-build5.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm113-tests1-linux64-build6.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-1-bm131-tests1-linux64-build4.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-2-bm131-tests1-linux64-build7.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-1-bm130-tests1-linux64-build6.txt:3
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-7-bm54-tests1-linux64-build0.txt:2
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-6-bm115-tests1-linux64-build5.txt:2
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-7-bm122-tests1-linux64-build7.txt:2
> mozilla-central_ubuntu64_vm-debug_test-mochitest-6-bm117-tests1-linux64-build3.txt:2
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-3-bm115-tests1-linux64-build13.txt:1
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-5-bm54-tests1-linux64-build4.txt:1

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

> 231 - devtools/client/debugger/test/mochitest/browser_dbg_watch-expressions-02.js
> 124 - /url/url-constructor.html
> 120 - /fetch/api/cors/cors-redirect-credentials-worker.html
> 120 - browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
> 104 - devtools/client/debugger/test/mochitest/browser_dbg_variables-view-edit-getset-01.js
> 81 - toolkit/components/extensions/test/mochitest/test_ext_alarms.html
> 72 - toolkit/components/extensions/test/mochitest/test_ext_i18n.html
> 52 - devtools/client/webconsole/test/browser_webconsole_output_02.js
> 52 - browser/components/extensions/test/browser/browser_ext_tabs_executeScript_good.js
> 49 - devtools/client/webconsole/test/browser_webconsole_output_06.js

[1] https://hg.mozilla.org/mozilla-central/annotate/b6683e141c47/dom/base/URL.cpp#l96

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I tried breakpointing on this warning in devtools/client/debugger/test/mochitest/browser_dbg_watch-expressions-02.js.  What's happening is that we're getting a call to the constructor with no base URL passed in and the actual url string is "null". 

The JS callstack is:

0 TabSources.prototype._isMinifiedURL(aURL = null) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/utils/TabSources.js":262]
    this = [object Object]
1 TabSources.prototype.source( = [object Object]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/utils/TabSources.js":181]
    this = [object Object]

So we could presumably get rif of most of the devtools warnings by not trying to |new URL(null)|.

I looked at fetch/api/cors/cors-redirect-credentials-worker.html and this warning is catching a bug in that test!  Filed https://github.com/w3c/web-platform-tests/issues/2801

The url/url-constructor.html test purposefully passes in invalid URI strings to thest their handling.

I suppose we could stop warning here, but it might be worth fixing the obvious issues from above and seeing how many instances of this warning remain, since it's already caught one bug just there.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> So we could presumably get rif of most of the devtools warnings by not trying to |new URL(null)|.

"get rid", I mean.

Comment 3

[Eric Rahm [:erahm]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> So I tried breakpointing on this warning in
> devtools/client/debugger/test/mochitest/browser_dbg_watch-expressions-02.js.
> What's happening is that we're getting a call to the constructor with no
> base URL passed in and the actual url string is "null". 
> 
> The JS callstack is:
> 
> 0 TabSources.prototype._isMinifiedURL(aURL = null)
> ["resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/server/actors/utils/TabSources.js":262]
>     this = [object Object]
> 1 TabSources.prototype.source( = [object Object])
> ["resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/server/actors/utils/TabSources.js":181]
>     this = [object Object]
> 
> So we could presumably get rif of most of the devtools warnings by not
> trying to |new URL(null)|.
> 
> I looked at fetch/api/cors/cors-redirect-credentials-worker.html and this
> warning is catching a bug in that test!  Filed
> https://github.com/w3c/web-platform-tests/issues/2801
> 
> The url/url-constructor.html test purposefully passes in invalid URI strings
> to thest their handling.
> 
> I suppose we could stop warning here, but it might be worth fixing the
> obvious issues from above and seeing how many instances of this warning
> remain, since it's already caught one bug just there.

I'm on the fence on this, we already throw an exception on null so warning doesn't make a ton of sense. It's also not outrageous to provide a bad url to the constructor if you know it will throw. Considering the warning shows up in 447 tests that's a bit involved to "fix" each one.

On the other hand it did catch a wpt bug.

For now I'll put up a fix for devtools and see how much that reduces that warning count.

Comment 4

[Eric Rahm [:erahm]]

Fixing that dev tools bit took care of 1400 warnings.

Comment 5

[Eric Rahm [:erahm]]

This is currently the #1 most verbose warning during testing. I took a look at some of the more verbose tests (web extensions) and it looks like a lot are coming from the following pattern [1]:

> try {
>   new URL(notSureIfIsValid)
> } catch {
>   // okay it's not valid do something
> }
> // otherwise do something else

Basically scripts relying on the URL class to take care of URL logic for them, which makes sense. I'm leaning towards removing the warning at this point.

Latest details:

> 3626 WARNING: 'NS_FAILED(rv)', file dom/url/URL.cpp, line 246

This warning [2] shows up in the following test suites:

>    517 - desktop-test-linux64/debug-mochitest-browser-chrome-2 bc2
>    506 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-4 bc4
>    454 - desktop-test-linux64/debug-mochitest-devtools-chrome-10 dt10
>    264 - desktop-test-linux64/debug-mochitest-10 10
>    264 - desktop-test-linux64/debug-mochitest-e10s-10 10
>    237 - desktop-test-linux64/debug-mochitest-devtools-chrome-9 dt9
>    192 - desktop-test-linux64/debug-mochitest-devtools-chrome-7 dt7
>    170 - desktop-test-linux64/debug-mochitest-devtools-chrome-8 dt8
>    159 - desktop-test-linux64/debug-mochitest-devtools-chrome-2 dt2
>    121 - desktop-test-linux64/debug-mochitest-devtools-chrome-5 dt5
>    114 - desktop-test-linux64/debug-mochitest-devtools-chrome-6 dt6
>    111 - desktop-test-linux64/debug-mochitest-chrome-3 c3
>     85 - desktop-test-linux64/debug-mochitest-devtools-chrome-1 dt1
>     76 - desktop-test-linux64/debug-mochitest-devtools-chrome-3 dt3
>     60 - desktop-test-linux64/debug-mochitest-devtools-chrome-4 dt4
>     56 - desktop-test-linux64/debug-web-platform-tests-10 10
>     56 - desktop-test-linux64/debug-web-platform-tests-e10s-10 10
>     41 - desktop-test-linux64/debug-mochitest-clipboard cl
>     38 - desktop-test-linux64/debug-mochitest-clipboard-e10s cl
>     15 - desktop-test-linux64/debug-mochitest-chrome-1 c1
>     13 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-3 bc3
>     10 - desktop-test-linux64/debug-mochitest-a11y a11y
>      7 - desktop-test-linux64/debug-mochitest-e10s-7 7
>      7 - desktop-test-linux64/debug-mochitest-7 7
>      6 - desktop-test-linux64/debug-web-platform-tests-e10s-4 4
>      6 - desktop-test-linux64/debug-mochitest-6 6
>      6 - desktop-test-linux64/debug-mochitest-e10s-6 6
>      6 - desktop-test-linux64/debug-web-platform-tests-4 4
>      6 - desktop-test-linux64/debug-mochitest-jetpack JP
>      3 - desktop-test-linux64/debug-mochitest-browser-chrome-1 bc1
>      3 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-6 bc6
>      3 - desktop-test-linux64/debug-mochitest-browser-chrome-4 bc4
>      3 - desktop-test-linux64/debug-mochitest-2 2
>      3 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-1 bc1
>      3 - desktop-test-linux64/debug-mochitest-browser-chrome-3 bc3
>      2 - desktop-test-linux64/debug-mochitest-browser-chrome-7 bc7
>      2 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-7 bc7
>      1 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-2 bc2

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

>     60 -        browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
>     60 - [e10s] browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
>     54 -        /url/url-constructor.html
>     54 - [e10s] /url/url-constructor.html
>     42 -        toolkit/components/extensions/test/mochitest/test_ext_cookies_permissions.html
>     29 - [e10s] browser/components/extensions/test/browser/browser_ext_tabs_executeScript_good.js
>     29 -        browser/components/extensions/test/browser/browser_ext_tabs_executeScript_good.js
>     25 - [e10s] browser/components/extensions/test/browser/browser_ext_runtime_openOptionsPage.js
>     25 -        browser/components/extensions/test/browser/browser_ext_runtime_openOptionsPage.js
>     24 -        toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html

[1] https://dxr.mozilla.org/mozilla-central/rev/ffac2798999c5b84f1b4605a1280994bb665a406/toolkit/components/extensions/Schemas.jsm#320-351
[2] https://hg.mozilla.org/mozilla-central/annotate/e78975b53563/dom/url/URL.cpp#l246

Comment 6

[Eric Rahm [:erahm]]

Attachment: Remove warning from URL constructor

Remove warning if constructing a new URI fails. An exception is still thrown on failure.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8779023 [details] [diff] [review]
Remove warning from URL constructor

What's changed since my earlier comments in this bug?  Again, note that a quick look back then actually showed this warning catching bugs...

Comment 8

[Eric Rahm [:erahm]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8779023 [details] [diff] [review]
> Remove warning from URL constructor
> 
> What's changed since my earlier comments in this bug?  Again, note that a
> quick look back then actually showed this warning catching bugs...

(In reply to Boris Zbarsky [:bz] from comment #1)
> 
> I suppose we could stop warning here, but it might be worth fixing the
> obvious issues from above and seeing how many instances of this warning
> remain, since it's already caught one bug just there.

We fixed the devtools "bug", you filed a bug for WPT (I'm not sure if that got fixed), the URL constructor tests got a pass because that's what they're meant to do, and now the most prevalent appears to be coming from usage by web extensions which in this case appears to be a legitimate usage (trying to create a possibly bad URL / ensuring the URL is relative) causing a warning [1] . If you think there's a better way for web extensions to do this I'd be happy to craft a patch for that so we can find other potential bugs, but as-is I don't think we're going to find additional legitimate bugs without changing the web extensions behavior.

[1] https://dxr.mozilla.org/mozilla-central/rev/ffac2798999c5b84f1b4605a1280994bb665a406/toolkit/components/extensions/Schemas.jsm#320-351

Comment 9

[Boris Zbarsky [:bzbarsky]]

I wonder whether it's worth just suppressing the warning when we're inside webextension script...

But yes, the other option is to remove it entirely and hope people don't write crappy tests.  Please check with baku about what he wants done here.

Comment 10

[Eric Rahm [:erahm]]

baku, do you have any opinions on the right path forward?

Comment 11

[Andrea Marchesini [:baku]]

I'm OK to remove this warning. The URL CTOR can be used to check if a string is a valid URL, and writing a stderr warning is not useful information here.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8779023 [details] [diff] [review]
Remove warning from URL constructor

Review of attachment 8779023 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/url/URL.cpp
@@ +238,5 @@
>  
>    nsCOMPtr<nsIURI> uri;
>    nsresult rv = NS_NewURI(getter_AddRefs(uri), aURL, nullptr, aBase,
>                            nsContentUtils::GetIOService());
> +  if (NS_FAILED(rv)) {

Write a comment here explaining why we don't use NS_WARN_IF here.

Comment 13

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abeb904ff83c87299f6138576290449eb21df246
Bug 1262948 - Remove warning from URL constructor. r=baku

Comment 14

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

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

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Eric, is it something you would like to uplift to aurora / beta? Thanks

Comment 16

[Eric Rahm [:erahm]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #15)
> Eric, is it something you would like to uplift to aurora / beta? Thanks

No need to uplift, we mainly monitor warnings on m-c builds.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

ok, thanks!