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

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

(Blocks: 1 bug)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 fixed)

Details

(Whiteboard: [tw-dom] btpp-fixlater)

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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
Whiteboard: [tw-dom] btpp-fixlater
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.
> So we could presumably get rif of most of the devtools warnings by not trying to |new URL(null)|.

"get rid", I mean.
(Assignee)

Comment 3

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

Updated

3 years ago
Depends on: 1266256
(Assignee)

Comment 4

3 years ago
Fixing that dev tools bit took care of 1400 warnings.
(Assignee)

Comment 5

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

Comment 6

2 years ago
Created attachment 8779023 [details] [diff] [review]
Remove warning from URL constructor

Remove warning if constructing a new URI fails. An exception is still thrown on failure.
Attachment #8779023 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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...
Attachment #8779023 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

2 years ago
(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
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 10

2 years ago
baku, do you have any opinions on the right path forward?
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
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.
Attachment #8779023 - Flags: review+

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/abeb904ff83c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Eric, is it something you would like to uplift to aurora / beta? Thanks
status-firefox48: affected → wontfix
status-firefox49: --- → affected
status-firefox50: --- → affected
Flags: needinfo?(erahm)
(Assignee)

Comment 16

2 years ago
(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.
Flags: needinfo?(erahm)
ok, thanks!
status-firefox49: affected → wontfix
status-firefox50: affected → wontfix
You need to log in before you can comment on or make changes to this bug.