1268595 - WindowsPreviewPerTab code causes very frequent warnings in the console: Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo)

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[:Gijs (he/him)]

STR:

1. on an up-to-date tree, run:

./mach mochitest browser/base/content/test/urlbar/browser_urlbarHashChangeProxyState.js

ER:

no warnings unrelated to what's being tested

AR:

10 lines of output along the lines of:

2 INFO TEST-INFO | (browser-test.js) | Console message: Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo). Please create channel using NetUtil.newChannel()

5 INFO Console message: Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo). Please create channel using NetUtil.newChannel()
6 INFO Console message: Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo). Please create channel using NetUtil.newChannel()
7 INFO Console message: Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo). Please create channel using NetUtil.newChannel()
8 INFO Console message: Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo). Please create channel using NetUtil.newChannel()

None of these are properly reported errors, so there are no stacks and I have no idea what's triggering it.

Christoph, can we make these things have stacks and fix them and/or at least shut these things up somehow?

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Gijs, unfortunately we can't have an 'hard' assertion there for backwards compatibility. Addons might not have updated their implementation to provide loadinfos for all of their channels. Before we landed Bug 1257930 however I pushed such an 'hard' assertion to TRY to make sure Gecko is up to date (see [1]) and does not produce any such console warnings.

When running browser_urlbarHashChangeProxyState.js locally I don't see those console warnings you are mentioning in comment 0, which might indicate that the problem might have been introduced by Predictor::Prefetch() within Bug 1016628. Have you seen those warnings before 04-26 as well?

We are tracking a similar issue about numerous NS_WARNINGs triggered by a missing loadinfo over in Bug 1267591. Those warnings are triggered by Predictor::Prefetch() [2]. Nick is on it and will fix those warnings over in Bug 1267058.

I suppose we could mark this bug as a duplicate of Bug 1267058.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1016628
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/Predictor.cpp#1335

Comment 2

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Gijs, unfortunately we can't have an 'hard' assertion there for backwards
> compatibility. Addons might not have updated their implementation to provide
> loadinfos for all of their channels.

No, but if you use Cu.reportError from JS, you get a stack. I don't know what this code is using, but it does not provide a stack. Of course, if the callers wind through C++/XPCOM, I don't know that the stack will be that useful...

> Before we landed Bug 1257930 however I
> pushed such an 'hard' assertion to TRY to make sure Gecko is up to date (see
> [1]) and does not produce any such console warnings.

It's odd that that didn't trigger, then. Are you effectively saying it's unexpected that I see so many of these warnings ? 

> When running browser_urlbarHashChangeProxyState.js locally I don't see those
> console warnings you are mentioning in comment 0, which might indicate that
> the problem might have been introduced by Predictor::Prefetch() within Bug
> 1016628. Have you seen those warnings before 04-26 as well?

I don't remember when I started seeing them. I would test but my home internet is down (typing through tethering my mbp on my cellphone) and so downloading an old build isn't really particularly easy... I did just notice that I don't see the errors on OS X, but I do on Windows. Builds are from the same bits of fx-team (same rev (changeset: e7570b6d28aa)
, though some unrelated stuff from yours truly locally on top of that).

> We are tracking a similar issue about numerous NS_WARNINGs triggered by a
> missing loadinfo over in Bug 1267591. Those warnings are triggered by
> Predictor::Prefetch() [2]. Nick is on it and will fix those warnings over in
> Bug 1267058.

Right, but these aren't NS_WARNINGS... they're Services.console.logStringMessage, which shows up in opt builds and so on...

> I suppose we could mark this bug as a duplicate of Bug 1267058.

I don't really understand why this is a dupe if that bug is fixing NS_WARNINGs and this bug is about Services.console.logStringMessage...

Comment 3

[:Gijs (he/him)]

OK, having used Cu.reportError, I know what is causing this and why it doesn't happen on OS X: It's the WindowsPreviewPerTab code. I'm fairly sure that means it's not a dupe of bug 1267058.

Comment 4

[:Gijs (he/him)]

I don't really understand how to fix this, because we call ioSvc.newChannelFromURI2 already in _imageFromURI, so it's not clear to me what's wrong with that channel. Do we just pass null too much? What should we be passing instead?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch (back May 3rd) from comment #3)
> OK, having used Cu.reportError, I know what is causing this and why it
> doesn't happen on OS X: It's the WindowsPreviewPerTab code. I'm fairly sure
> that means it's not a dupe of bug 1267058.

Now I see what's going on. And I also know what will fix those issues. WindowsPreviewPerTab should not use SEC_NORMAL as a security flag anymore, but rather a real security flag so we can call asyncOpen2().

I already landed that conversion last night [1]; should hit mozilla-central later today. In fact those errors are unexpected and we shouldn't see any of those warnings on TreeHerder. To be clear, we shouldn't see any NS_WARNINGs that there is no loadInfo, nor should we see any console.logStringMessage warnings related to loadInfo on Treeherder.

Now, my final question. If we would use Cu.reportError and an addon would call asyncFetch() without providing the right security flags and we Cu.reportError would break that addon, right? Just logging to the console would *not* break that addon. If I am wrong, then please let me know and I'll update that.

Hopefully I have answered all your questions!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1255499

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Sorry, mid-air collision reverted the flags - should be back to what Gijs set them.

Comment 7

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> (In reply to :Gijs Kruitbosch (back May 3rd) from comment #3)
> > OK, having used Cu.reportError, I know what is causing this and why it
> > doesn't happen on OS X: It's the WindowsPreviewPerTab code. I'm fairly sure
> > that means it's not a dupe of bug 1267058.
> 
> Now I see what's going on. And I also know what will fix those issues.
> WindowsPreviewPerTab should not use SEC_NORMAL as a security flag anymore,
> but rather a real security flag so we can call asyncOpen2().
> 
> I already landed that conversion last night [1]; should hit mozilla-central
> later today. In fact those errors are unexpected and we shouldn't see any of
> those warnings on TreeHerder. To be clear, we shouldn't see any NS_WARNINGs
> that there is no loadInfo, nor should we see any console.logStringMessage
> warnings related to loadInfo on Treeherder.

Great!

> Now, my final question. If we would use Cu.reportError and an addon would
> call asyncFetch() without providing the right security flags and we
> Cu.reportError would break that addon, right? Just logging to the console
> would *not* break that addon. If I am wrong, then please let me know and
> I'll update that.

No, add-ons won't break functionality-wise. They would if you threw an actual exception / returned a non-NS_OK thing off an XPCOM method. But none of those is the case here. Cu.reportError is just much better at actually providing a stack trace.

The only thing I can think of that makes Cu.reportError slightly worse for add-ons is that AMO reviewers will care more about these types of errors than about the logStringMessage ones, because they go in different categories in the browser console (logStringMessage provides a different severity message, and so they have a transparent background; Cu.reportError is reporting an error, so it gets a reddish background and looks more, um, severe, I guess).

On the other hand, Cu.reportError is also better for add-on authors because it provides a stack trace to what call is breaking something, so it's much easier to figure out what's broken than if we just logStringMessage but don't provide a stack (leaving the add-on author wondering "that's nice, but what call to asyncFetch broke this?", especially if it's through browser APIs that they call with maybe suboptimal parameters or something).

So I think we should change this to use Cu.reportError. Does that make sense?

> 
> Hopefully I have answered all your questions!
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1255499

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch (back May 3rd) from comment #7)
> So I think we should change this to use Cu.reportError. Does that make sense?

Thanks for the clarification. I didn't know that. Let's use Cu.reportError then.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Also adding a dependency to Bug 1255499.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1268595_use_cu_reporterror_within_netutil_asyncfetch.patch

Gijs, you are not accepting review requests - but this one can wait for you till you are back.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8747641 [details] [diff] [review]
bug_1268595_use_cu_reporterror_within_netutil_asyncfetch.patch

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

r=me!

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #11)
> r=me!

Thanks!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f930f51a9b0a

Comment 14

[Carsten Book [:Tomcat]]

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