991734 - Rare but important warnings got thrown out with the too-common-warning bathwater

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Bug 966692 turned off showing JS warning in the console by default.

Unfortunately, there are warnings we want web developers to see because they indicate serious bugs in their sites that are not actually "JS" warnings but are getting lumped in there by the console.  For example the warning about <meta charset> after the sniffing boundary...

This change also makes deprecation warnings not visible by default, which means it's compeletely pointless to warn about deprecations now.  That may significantly affect our ability to evolve the web platform.  That's assuming anyone was paying attention to those warnings to start with, of course.

I realize that some warnings (like the sourcemap thing that jquery hits) are just way too common and we want to not show them by default.  Are there examples other than the sourcemap thing?  I'd like to understand which subsystems exactly are producing too many warnings, and there is no real concrete info on that in bug 966692.

The one concrete thing that _is_ there is that nytimes.com has too many warnings.  When I just loaded it, the JS warnings I see there are:

1) Use of getPreventDefault() is deprecated.  Use defaultPrevented instead. 
2) The character encoding of a framed document was not declared. The document may appear different if viewed without the document framing it.

That doesn't seem particularly overwhelming, but if those warnings are too common we should in fact consider turning them off on the platform level...

I guess my point is that I would like us to make a distinction between "warnings that we want to warn about for pedantic reasons but are hit all the time" and "warnings that are not common but indicate a serious problem when present".  I'm happy to split things into different categories on the Gecko end to accomplish this, but that requires the UI to not lump those categories together (like it does now, with treating HTML parser warnings as "JS" warnings).

Thoughts?

If we do keep the devtools UI as-is, we should just remove all the infrastructure we have for deprecation warnings and such in Gecko, since it becomes completely pointless.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and looking back at builds before bug 966692, the vast majority of the spew was all the network bits.  This was mentioned in bug 966692 comment 6, and comment 7 seemed to agree that just disabling the network logs was the way to go, but then we landed something different from that proposal?

Comment 2

[Joe Walker [:jwalker]]

We would like to have a new tool which allows us to separate the JS console from warnings and errors. A 'lint' panel if you like. This would enable us to turn most things on by default.

I think this was also influenced by the tweet in Bug 953166 comment 0.

Not saying there aren't things we can improve though.

Comment 3

[(no longer active)]

Can we back out bug 966692 while this new tool is in the works?

Comment 4

[Mihai Sucan [:msucan]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #3)
> Can we back out bug 966692 while this new tool is in the works?

No need to backout. We can re-enable only JS warnings. Would that be fine?

Comment 5

[Boris Zbarsky [:bzbarsky]]

That would work for me as a stopgap, yes.

I _am_ pretty interested in reducing warning spew, and happy to do whatever platform bits would help with that in terms of categorizing things, removing deprecation warnings that are pointless (e.g. we have no plans to remove the feature), etc.

Comment 6

[Joe Walker [:jwalker]]

I wonder what the benefit of the "XUL box for %1$S element contained an inline %2$S child, forcing all its children to be wrapped in a block." is [1].

There is some evidence that it's confusing [2], maybe we could just re-word it.

[1]: https://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9445
[2]: https://stackoverflow.com/questions/13618306/what-is-this-css-error

Comment 7

[Boris Zbarsky [:bzbarsky]]

I believe the intent is to warn about a performance pitfall.  I think we should restrict that warning to chrome, not web pages, because web pages that are using XUL boxes at all are not going to care about performance details like that.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The intent was also to warn that the page in question was affected by a last-minute compat change we had to make at one pint as part of a security fix, if memory serves.

I'm fine with leaving it for Chrome, although at some point we should have a warning for Web content that XUL box is deprecated.  Maybe not yet, though.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #0) 
> 1) Use of getPreventDefault() is deprecated.  Use defaultPrevented instead. 
I'd be surprised if this was common. And the warning should be once per document.
Though, IIRC nytimes.com tends to load new iframes all the time.
Perhaps we should warn only in case of top level content document.

Comment 10

[Joe Walker [:jwalker]]

I raised bug 991928.

However given:

:bz
> I believe the intent is to warn ...

:dbaron
> ... if memory serves

I can't help but wonder if it's not an important warning, and just removing it wouldn't be a better idea.

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to Olli Pettay [:smaug] from comment #9)
> (In reply to Boris Zbarsky [:bz] from comment #0) 
> > 1) Use of getPreventDefault() is deprecated.  Use defaultPrevented instead. 
> I'd be surprised if this was common. And the warning should be once per
> document.

It is common because every web page using some versions of jQuery will spew the warning.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: bug991734-1.diff

This patch re-enables js warnings in the webconsole.

Comment 13

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky [:bz] from comment #0)
> I realize that some warnings (like the sourcemap thing that jquery hits) are
> just way too common and we want to not show them by default.  Are there
> examples other than the sourcemap thing?  I'd like to understand which
> subsystems exactly are producing too many warnings, and there is no real
> concrete info on that in bug 966692.

Just loaded cnn.com and cnet.com. I see 'Use of getUserData() or setUserData() is deprecated.  Use WeakMap or element.dataset instead.' is very common.


> The one concrete thing that _is_ there is that nytimes.com has too many
> warnings.  When I just loaded it, the JS warnings I see there are:
> 
> 1) Use of getPreventDefault() is deprecated.  Use defaultPrevented instead. 
> 2) The character encoding of a framed document was not declared. The
> document may appear different if viewed without the document framing it.
> 
> That doesn't seem particularly overwhelming, but if those warnings are too
> common we should in fact consider turning them off on the platform level...

Yes, or report these warnings only once per document, or even only once per top level document.


> I guess my point is that I would like us to make a distinction between
> "warnings that we want to warn about for pedantic reasons but are hit all
> the time" and "warnings that are not common but indicate a serious problem
> when present".  I'm happy to split things into different categories on the
> Gecko end to accomplish this, but that requires the UI to not lump those
> categories together (like it does now, with treating HTML parser warnings as
> "JS" warnings).
> 
> Thoughts?

Different categories in Gecko might make sense in some cases, but the webconsole UI cant get additional categories, for now. We've been discussing the situation and it would mean too much additional UI clutter. Joe mentioned an additional tool for js eval that we are planning - that will allow the webconsole to become a log-only tool. That will be better and I hope we will also improve the console UI.

Until then it would be really useful if Gecko could limit the number of messages it logs - and in particular the repeats. I don't think we should remove common deprecation warnings, but we should make sure they dont show so many times in a single page load.

We could also be more relaxed about js warnings and detect duplicates from different sources and lines (only for js warnings), directly in the webconsole UI. We already have all we need to do this.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I see 'Use of getUserData() or setUserData() is deprecated.  Use WeakMap or
> element.dataset instead.' is very common.

You must be using adblock plus?  Those sites don't use that API, but adblock plus sure does.

I wonder whether we should just turn off the userdata warning for chrome-touching-content for now.

> Yes, or report these warnings only once per document

That's already the case for getPreventDefault.  It's a once-per-document warning.

> but we should make sure they dont show so many times in a single page load.

All deprecation warnings are once-per-document, fwiw.  Are commonly really seeing them lots of times for a single page load?  It would have to be due to ad frames or something....

Comment 15

[Boris Zbarsky [:bzbarsky]]

One other note.  Having more control over category visibility does not necessarily mean more buttons in the console UI.  e.g. one possible breakdown might be to show "errors", "common warnings", "pedantic warnings" or something and map those onto categories in some way....

Comment 16

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

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

Comment 17

[Phil Ringnalda (:philor)]

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

Comment 18

[Mihai Sucan [:msucan]]

Should we get the patch in aurora as well?

Comment 19

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> > I see 'Use of getUserData() or setUserData() is deprecated.  Use WeakMap or
> > element.dataset instead.' is very common.
> 
> You must be using adblock plus?  Those sites don't use that API, but adblock
> plus sure does.
> 
> I wonder whether we should just turn off the userdata warning for
> chrome-touching-content for now.

Indeed. I was using abp but I had it disabled (I was expecting it doesnt interfere). Still chrome-touching-content warnings (and errors?) should show in browser console, not in the web console. Is there a bug open about this?


> > but we should make sure they dont show so many times in a single page load.
> 
> All deprecation warnings are once-per-document, fwiw.  Are commonly really
> seeing them lots of times for a single page load?  It would have to be due
> to ad frames or something....

Could be...


(In reply to Boris Zbarsky [:bz] from comment #15)
> One other note.  Having more control over category visibility does not
> necessarily mean more buttons in the console UI.  e.g. one possible
> breakdown might be to show "errors", "common warnings", "pedantic warnings"
> or something and map those onto categories in some way....

This is an idea that could work. If having js warnings enabled by default becomes a problem, we should do this.

Thanks!

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Should we get the patch in aurora as well?

Imo, yes.

> Still chrome-touching-content warnings (and errors?) should show in browser console

In this case the warning is associated with a node, not a script, and hence shows up as attached to that node's document...  I agree we should try to fix that (e.g. we should actually show a useful file/line for deprecation warnings, which we don't right now).  There is no bug on this that I know of.

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 8401948 [details] [diff] [review]
bug991734-1.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 966692
User impact if declined: as pointed out by bz in comment #0 some of the warnings we hid with the patch for bug 966692 are sufficiently important to display. if this patch doesnt land users can miss warnings about deprecated APIs.
Testing completed (on m-c, etc.): landed in fx-team and m-c.
Risk to taking this patch (and alternatives if risky): no risk, we just re-enable js warnings in the console, by default.
String or IDL/UUID changes made by this patch: none.

Comment 22

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> > Still chrome-touching-content warnings (and errors?) should show in browser console
> 
> In this case the warning is associated with a node, not a script, and hence
> shows up as attached to that node's document...  I agree we should try to
> fix that (e.g. we should actually show a useful file/line for deprecation
> warnings, which we don't right now).  There is no bug on this that I know of.

reported bug 992967

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d3ab72461f94