Closed Bug 1262948 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

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.
(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.
Depends on: 1266256
Fixing that dev tools bit took care of 1400 warnings.
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
Remove warning if constructing a new URI fails. An exception is still thrown on failure.
Attachment #8779023 - Flags: review?(bzbarsky)
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)
(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)
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+
https://hg.mozilla.org/mozilla-central/rev/abeb904ff83c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Eric, is it something you would like to uplift to aurora / beta? Thanks
Flags: needinfo?(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.
Flags: needinfo?(erahm)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.