Closed Bug 1228034 Opened 9 years ago Closed 6 years ago

Developer Toolbar Error Count Incorrect

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect, P2)

44 Branch
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: jhg, Unassigned)

References

Details

Attachments

(3 files)

Attached image screenshot
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

Have developer toolbar visible, in 44.0a2 (2015-11-25)


Actual results:

Error count is some large number (147, 6639, etc) and does not match the errors displayed in the console (1).


Expected results:

The error count should reflect the number displayed errors and vice/versa.
Note: all filter buttons in the console panel are enabled (I can post individual screenshots showing the state of each filter set if desired).
Component: Untriaged → Developer Tools: Graphic Commandline and Toolbar
I just updated to version 48.0a2 (2016-04-27) and this bug is still there.

The error count isn't just some large number: it grows infinitely as long as you have the developer toolbar open. It renders the error counter feature useless.

This happens also with a clean user profile just created and in --safe-mode.

For contrast:

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160427004105
Still present on 51.0a2 (2016-10-12) (32-bit)

Could this be related to bug 1125768 ?
Disabling e10s (about:preferences -> General -> Enable multi-process …) fixes the error count
I've just reproduced this on all current versions from 50.1.0 to Nightly 53.0a1 (2016-01-12). It's obviously a regression introduced with e10s.

Joe, can you please prioritize this? May this be something a beginner could fix or is the issue more complex?

Sebastian
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jwalker)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
See Also: → 1125768
Forwarding Sebastian's request from comment 5 to Patrick.
Flags: needinfo?(jwalker) → needinfo?(pbrosset)
As Sebastian said, this is obviously something related to e10s.
I don't know too much about the dev toolbar, but looking at the code I found this:

The developer toolbar main script: devtools\client\shared\developer-toolbar.js contains the DeveloperToolbar class. This class is responsible for the UI of the dev toolbar. So it is executed in the parent process of the browser, where the rest of the devtools UI lives too.
With e10s, it means this process does not have access to the child content process.
Now, this script makes use of the ConsoleServiceListener to track errors on the page.
This listener thing is defined in devtools/server/actors/utils/webconsole-listeners, which is an actor thing. This script is supposed to be executed on the server, so it has access to the child content process and can correctly track page errors from there.

So the large error count we're seeing in the dev toolbar actually corresponds to the errors that happen in the browser UI. These are errors that normally don't really show up anywhere I guess, but the dev toolbar (in e10s mode) tracks them.

Let's triage this as a P2, and I'll ping Mike to confirm my investigation and re-triage if needed, since he has worked on the dev toolbar a lot more than me.
Flags: needinfo?(pbrosset) → needinfo?(mratcliffe)
Priority: -- → P2
Adding to papercuts.
Blocks: 1312444
Flags: needinfo?(mratcliffe)
(In reply to Patrick Brosset <:pbro> from comment #8)
> So the large error count we're seeing in the dev toolbar actually
> corresponds to the errors that happen in the browser UI. These are errors
> that normally don't really show up anywhere I guess

They show up in the Browser Console, if I'm not mistaken.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #9)
> Adding to papercuts.

This bug here is about the GCLI, so, bug 1312444 seems to be unrelated to it.

Sebastian
Flags: needinfo?(mratcliffe)
Whiteboard: [papercut-mr]
(In reply to Sebastian Zartner [:sebo] from comment #10)
> (In reply to Patrick Brosset <:pbro> from comment #8)
> > So the large error count we're seeing in the dev toolbar actually
> > corresponds to the errors that happen in the browser UI. These are errors
> > that normally don't really show up anywhere I guess
> 
> They show up in the Browser Console, if I'm not mistaken.
> 
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #9)
> > Adding to papercuts.
> 
> This bug here is about the GCLI, so, bug 1312444 seems to be unrelated to it.
> 
> Sebastian

By the letter you are right... I was using that bug to keep track of the papercuts I am trying to fix. I am now using the whiteboard tag [papercut-mr] instead.
No longer blocks: 1312444
Flags: needinfo?(mratcliffe)
The problem is this code:

```
  let window = tab.linkedBrowser.contentWindow;
  let listener = new ConsoleServiceListener(window, {
    onConsoleServiceMessage: this._onPageError.bind(this, tabId),
  });
```

Because we are in E10S mode tab.linkedBrowser.contentWindow is null.
@nchevobbe I will assign this to you for now but feel free to assign it back when you have looked into the Web Console side of things. I am only dealing with papercuts at the moment and this is turning into quite the rabbit hole.

The main part of this issue is the Web Console's ConsoleServiceListener Service. In a non-E10S world we used to be able to compare windows as the service does but now that the content window is unavailable we need to depend on window IDs.

devtools/client/shared/developer-toolbar.js

1. In DeveloperToolbar.prototype._initErrorsCount we attempt to get the content window and turn out with null due to E10S.
2. We then register a new ConsoleServiceListener along with window, which is null due to E10S.
3. Now when an error is triggered the error is passed to DeveloperToolbar.prototype._onPageError().
4. This checks whether the error should be displayed and then calls DeveloperToolbar.prototype._updateErrorsCount(), which updates the number of error on the GCLI button.

devtools/server/actors/utils/webconsole-listeners.js:69

1. ConsoleServiceListener.prototype.observe() is the ConsoleServiceListener's listener and we passed in null because window is null due to E10S.
2. Because window is null we bypass the bulk of observe() and count all errors produced by the browser rather than just the content window errors.

The problem:

Because we have no access to the content window from chrome code, observe() cannot depend on a window being passed in to filter the messages. The obvious solution is to change this to work with both windows and window IDs. This way it would work if a chrome window was passed in or a content window ID.

One further problem is that message.innerWindowID and message.outerWindowID are both 0 inside observe(). I believe this is because the nsConsoleMessage is converted to an nsIScriptError and it has no innerWindowID or outerWindowID properties.

STR

1. Apply the WebConsoleError patch (it is only 2 lines so feel free to do it manually).
2. Run the test case.
3. Press <shift><f2> to open GCLI.

You will see that the error count is too high in GCLI (it should only be one but it includes all browser errors in Chrome and content).

In your console (i.e. bash) you will see that the following has been logged along with all the other browser errors:
Object
    - QueryInterface = function QueryInterface() { [native code] }
    - logLevel = 3
    - timeStamp = 1484574442401
    - message = [JavaScript Error: "ReferenceError: xxx is not defined" {file: "file:///Users/ratcliffes/Desktop/cookie/errors.html" line: 9}]
    - toString = function toString() { [native code] }
    - debug = 0
    - info = 1
    - warn = 2
    - error = 3
    - errorMessage = ReferenceError: xxx is not defined
    - sourceName = file:///Users/ratcliffes/Desktop/cookie/errors.html
    - sourceLine =
    - lineNumber = 9
    - columnNumber = 9
    - flags = 2
    - category = content javascript
    - outerWindowID = 0
    - innerWindowID = 0
    - isFromPrivateWindow = false
    - stack = undefined
    - errorMessageName =
    - init = function init() { [native code] }
    - initWithWindowID = function initWithWindowID() { [native code] }
    - errorFlag = 0
    - warningFlag = 1
    - exceptionFlag = 2
    - strictFlag = 4
    - infoFlag = 8

The innerWindowID and outerWindowID are both 0 so there is no way to match this with the current content Window.
Flags: needinfo?(chevobbe.nicolas)
Assignee: nobody → chevobbe.nicolas
Filter on Brobdingnagian.
Whiteboard: [papercut-mr] → [todo-mr]
Any progress with this? The behavior change is very annoying when one is coming from v52 v53 beta. :/
I might have some time this week to look at this
Sorry to bug you, any chance this could be sorted before v53 hits stable?
Let me chime in as well as FF53 is out with the same unresolved issue; it would be nice if someone fixed this and I could use the error count again (this page shows 900 errors on my setup as of now, and many pages show in access of half a million).
There was anotherrelated issue reported on Stack Overflow:

http://stackoverflow.com/q/43893261/432681

Nicolas, could you find some time to investigate this further?

Sebastian
> Nicolas, could you find some time to investigate this further?

Unfortunately, no, I'm pretty busy these days, and I can't really say when I'll be able to look at this
Flags: needinfo?(nchevobbe)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #22)
> > Nicolas, could you find some time to investigate this further?
> 
> Unfortunately, no, I'm pretty busy these days, and I can't really say when
> I'll be able to look at this

Sad to hear that! So, I unassign you in order to give others the chance to jump in. Feel free to reassign it to yourself once you finished your other tasks.

Sebastian
Assignee: nchevobbe → nobody
Late in the game for 54, moving to something for 55.
Any news on this? Still happens as you already know with v56 and is very annoying :/
I agree that this bug is quite annoying and I just reproduced it in Nightly 57.0a1 (2017-08-23).

I fear that the DevTools team is too busy at the moment preparing things for the 57 release[1], which will be a huge one. But let's ask Mike if he can jump in, as he did a detailed investigation of the problem in comment 14.

Sebastian

[1] https://www.cnet.com/how-to/see-the-future-firefox-right-now/
Yes, it annoys me too.

The issue is that the toolbar logs all of the exceptions for all tabs and, as far as I can gather, there is currently no way for us to identify which errors come from which tab in order to filter them.

Of course, if anybody has time to take a look at this then please feel free.
Flags: needinfo?(mratcliffe)
I wish I was familiar with Firefox's codebase in order to try to fix this. :/
(In reply to xhmikosr@gmail.com from comment #28)
> I wish I was familiar with Firefox's codebase in order to try to fix this. :/

The related code is here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/developer-toolbar.js

Though regarding comment 14 it sounds to be something rather complicated to fix.

Mike, as the Web Console already filters the messages per tab and message type, I am wondering if that logic couldn't be reused for the Developer Toolbar.

Sebastian
(In reply to Sebastian Zartner [:sebo] from comment #29)
> Mike, as the Web Console already filters the messages per tab and message
> type, I am wondering if that logic couldn't be reused for the Developer
> Toolbar.
> 

nchevobbe is over the Web Console these days so maybe he could answer your question?
Flags: needinfo?(nchevobbe)
I had a quick look today, but this won't be a small fix.
The thing is that in the console we use the WebConsole client to listen to new messages, where the developer-toolbar directly creates a ConsoleServiceListener object with a window that we don't have anymore.

So the fix would be either being able to pass a windowId to the ConsoleServiceListener, which requires some serious changes since we would have to support both listening on a window and a windowId.
The other option would be to do things more like the console, i.e. create a webConsole client and listen for "pageError" event.
Flags: needinfo?(nchevobbe)
It only happened when the window/tab opened using URL, if a new BLANK TAB opened and then type/paste the URL the issue disappeared!
Gonna remove the regression keyword since we've been shipping this bug since 44.
The issue continues, unfortunately, in FF 57.0.1; every single Web page shows large numbers of errors, regardless of how the page is initiated. Just opening a new blank tab gives errors each new tab opened showing larger numbers.
Yup, unfortunately.

At this point, since the counter is so broken, even removing it, might be better :/.
But it's so difficult to count red lines on console?
One of Firefox's pros is having the developer toolbar always open as an indicator of errors. If that indicator is wrong, it beats the point. Plus it's also misleading info, since the counter's value doesn't represent the console errors anymore.
People who are bit by this might enjoy the javascript-errors addon https://addons.mozilla.org/en-US/firefox/addon/javascript-errors/ , which was just published for Firefox.
Product: Firefox → DevTools
See Also: → 1469832
The reasons for why the Developer Toolbar is removed are outlined in https://groups.google.com/d/msg/mozilla.dev.developer-tools/0WoBr_HFLAA/TbkOTqYtBgAJ. The work on removing it is tracked in bug 1429421.

I'm sad about the removal, too, though the decision to remove the toolbar is unrelated to this bug. Actually, the error counter is one of the features that is supposed to be kept, as mentioned in the linked sites. I have therefore created bug 1469832, so it's added as a badge to the toolbar button.

Sebastian
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Too bad for the aforementioned decision. One of the things that personally made me not switch to Chrome was this: the ability to have the developer toolbar open by default.

Hopefully an extension will show up.
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: