1613367 - Calls to Cu.reportError() from chrome JS shouldn't be treated as "web content" messages

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1561930 +++

It seems bug 1561930 has regressed; I'm seeing the same thing on current nightly again. This is frustrating because the checkbox is less visible now (pushed into the menu) so it was less obvious I wasn't even seeing the content error messages; the checkbox used to be in the browser console toolbar which at least made it obvious the console was hiding messages.

This issue affects both the browser console and the (multiprocess) browser toolbox.

What were you doing?

1. Open Firefox with a clean profile
2. Open regular devtools
3. Enable chrome/add-on debugging in the devtools settings
4. open the browser console (accel-shift-j)
5. Run Cu.reportError("Hello");

What happened?

The hello message was not visible. It is only visible once you tick "Show content messages".

What should have happened?

The hello message should have been visible. It did not come from content and should not be filtered by that checkbox.

Comment 1

[:Harald Kirschner :digitarald]

Nicolas, without digging too deep, do you have any idea what changed here with error reporting in Browser Console?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

no I don't. The flag is set on the platform side.
Eden might have an idea.

We can also run mozregression

Comment 3

[Bogdan Maris, Desktop QA]

Attachment: Browser console.png

Hm I'm trying to repro this in order to run mozregression bug for some reason I can't seem to reproduce this in the first place (or maybe I'm missing something). I do get the Hello message displayed in latest Nightly and also the same output on builds before bug 1561930 was fixed.

Comment 4

[:Gijs (he/him)]

Hm, that's odd. I can no longer reproduce with the steps from comment #0. I filed this around the same time as bug 1613367, and so I suspect my original Cu.reportError was somewhere in contentAreaUtils.js, but I cannot reproduce with that either.

However, when toggling "Show Content Messages" on a clean profile, the errors from RemoteSettingsClient.jsm appear/disappear (Unknown Collection "main/message-groups"), which is still confusing - those are errors from chrome-privileged code, in the parent process. So I suspect there is an issue here, but we probably need better steps - we could look for a regression window for those messages but they were probably introduced by a change in the new tab page... But perhaps using those messages we can figure out why those errors aren't showing up without having "Show Content Messages" ticked? Nicolas?

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Eden, who did the work on the platform side would know better

Comment 6

[Eden Chuang[:edenchuang]]

Sorry for the late response.

There is a known issue that if a ScriptError is initialized with aInnerWindow Id 0, we might distinguish the error from content context, but it should be from chrome context.

Context recognization logic is checking if the principal of the inner window is system principal or not.
However, if ScriptError is initialized with a 0 innerWindow Id, we have no principal for judgment. And it would be considered from content context now.

I guess the bug here might meet the situation.

However, it is hard to distinguish every case that ScriptError with 0 innerWindowId in the ScriptError constructor.
So there is an optional parameter isFromChromeContext for ScriptError creation for the caller to specify if the ScriptError is from chrome context.

Comment 7

[:Gijs (he/him)]

(In reply to Eden Chuang[:edenchuang] from comment #6)

However, it is hard to distinguish every case that ScriptError with 0 innerWindowId in the ScriptError constructor.
So there is an optional parameter isFromChromeContext for ScriptError creation for the caller to specify if the ScriptError is from chrome context.

I'm sure this is a dumb question, but: I would have expected that errors from the parent process without an innerwindowid should automatically be assumed to be from chrome rather than content. That is, I would have assumed that if there is no innerWindowId, the first thing we'd check would be XRE_IsParentProcess(). Is that not possible for some reason?

Comment 8

[Eden Chuang[:edenchuang]]

Unfortunately this is not true.

https://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#4119

There are cases the child process sends the error information to the parent process, then constructs the ScriptError in the parent process.
And ContentChild::SendScriptError here with innerWindowId 0

https://searchfox.org/mozilla-central/source/dom/ipc/ContentChild.cpp#529-530

So if we just determine the XRE_IsParentProcess() && innerWindowId == 0 in nsScriptError constructor, we will might treat it from chrome context but it is not.

Comment 9

[:Gijs (he/him)]

(In reply to Eden Chuang[:edenchuang] from comment #8)

Unfortunately this is not true.

:-(

https://searchfox.org/mozilla-central/source/dom/ipc/ContentChild.cpp#529-530

Could we add the aFromChromeContext bool as a member of nsIConsoleMessage so it's always available, and set it to true inside Cu.reportError and when we get errors from workers / other chrome global contexts that have no window id, but have system principal (like, apparently, the remote settings case from comment #4 ) ?

Comment 10

[Eden Chuang[:edenchuang]]

It seems to be doable, will write a patch for it.

Comment 11

[Eden Chuang[:edenchuang]]

Attachment: Set Cu.reportError message as from chrome context if innerWindowId is 0

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db8b8c03eb66
Set Cu.reportError message as from chrome context if innerWindowId is 0 r=baku

Comment 13

[Cosmin Sabou [:CosminS]]

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