1763367 - HttpBaseChannel::MaybeFlushConsoleReports uses the wrong windowID

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Tom Schuster (MoCo)]

While working on adding console logging for bug 1763073 I noticed that none of those actually appeared in the WebConsole. The issue seems to be caused by this:

FlushReportsToConsole(nsContentUtils::GetInnerWindowID(aLoadGroup), aAction);

When the console message is missing nsContentUtils::GetInnerWindowID(aLoadGroup) returns 0. This seems to be caused by the first call to GetNotificationCallbacks somehow failing in that function.

I think we could just change HttpBaseChannel::MaybeFlushConsoleReports to directly use mLoadInfo->GetInnerWindowID(), but maybe there is some reason why we want to use the LoadGroup. Using the InnerWindowID from LoadInfo made the error messages show up in my testing.

Comment 1

[Stefan Popa]

Attach a log file please!!!!

Comment 2

[Tom Schuster (MoCo)]

Attachment: Bug 1763367 - Flush HttpBaseChannel console reports using the WindowID. r?dragana

Comment 3

[Tom Schuster (MoCo)]

I don't really know what the right fix here would be, but the current code is definitely sub-optimal, because it uses innerWindowId 0 even when the right ID is known.

Comment 4

[Dragana Damjanovic [:dragana]]

I do not mind using mLoadInfo instead of LoadGroup, but I am interested to know why LoaGroup fails to return innerWindowId. I am concerned that than may have an effect on the thing and I would like to investigate it.
Can you figure out why nsContentUtils::GetInnerWindowID(aLoadGroup) returns 0?

Comment 5

[Tom Schuster (MoCo)]

The call to GetNotificationCallbacks seems to fail because it either a nullptr or some other failure. Do you want me to dig deeper into why that happens?

Comment 6

[Tom Schuster (MoCo)]

It seems like with this change we are now logging other previously missed console messages to the web console. One of these try failures is in devtools/client/webconsole/test/browser/browser_webconsole_warning_group_content_blocking.js.
I am not sure what the best fix would be testStorageAccessBlockedGrouping calls checkConsoleOutputForWarningGroup which checks all console messages, but finds 4 unexpected ones:

Cookie “name=value” has been rejected as third-party.

(I am not sure how useful that message is in combination with other existing warnings, it seems a bit spamy)

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

So, if you want to get unblocked quickly, you could do

diff --git a/devtools/client/webconsole/test/browser/browser_webconsole_warning_group_content_blocking.js b/devtools/client/webconsole/test/browser/browser_webconsole_warning_group_content_blocking.js
--- a/devtools/client/webconsole/test/browser/browser_webconsole_warning_group_content_blocking.js
+++ b/devtools/client/webconsole/test/browser/browser_webconsole_warning_group_content_blocking.js
@@ -178,6 +178,7 @@ async function testStorageAccessBlockedG
   const now = Date.now();
 
   await clearOutput(hud);
+  await setFilterState(hud, { text: "-has been rejected" });

This will filter out all those "new" messages
Otherwise, you might want to integrate those rejected cookies warning in the arrays of the calls to checkConsoleOutputForWarningGroup, but I don't know if the order is stable enough.
The warnings were added in Bug 1596741, not too long ago, so I guess they made sense.

Comment 8

[Tom Schuster (MoCo)]

The warnings were added in Bug 1596741, not too long ago, so I guess they made sense.

I wonder if these warnings ever actually showed up in the console before. Either we had some kind of regression or we didn't always show those messages.

Comment 9

[Tom Schuster (MoCo)]

Attachment: Bug 1763367 - Filter out unexpected messages. r?nchevobbe

Depends on D143039

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd28d8497f99
Flush HttpBaseChannel console reports using the WindowID. r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/9b27117a24c8
Filter out unexpected messages. r=nchevobbe

Comment 11

[Cristian Tuns]

Backed out as requested by evilpie on element

• Backout link

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f66ea577c6b
Flush HttpBaseChannel console reports using the WindowID. r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/83e086e3057e
Filter out unexpected messages. r=nchevobbe

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9f66ea577c6b
https://hg.mozilla.org/mozilla-central/rev/83e086e3057e