Closed Bug 1763367 Opened 3 months ago Closed 1 month ago

HttpBaseChannel::MaybeFlushConsoleReports uses the wrong windowID

Categories

(Core :: Networking, defect, P3)

defect

Tracking

()

RESOLVED FIXED
102 Branch
Tracking Status
firefox102 --- fixed

People

(Reporter: tschuster, Assigned: tschuster)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(2 files)

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.

Severity: -- → S4
Priority: -- → P3
Whiteboard: [necko-triaged]
Assignee: nobody → tschuster
Status: NEW → ASSIGNED

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.

Flags: needinfo?(dd.mozilla)

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?

Flags: needinfo?(dd.mozilla)

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?

Attachment #9271072 - Attachment description: WIP: Bug 1763367 - For demo purposes → Bug 1763367 - Flush HttpBaseChannel console reports using the WindowID. r?dragana

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)

Flags: needinfo?(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.

Flags: needinfo?(nchevobbe)

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.

Depends on D143039

Pushed by tschuster@mozilla.com:
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

Backed out as requested by evilpie on element

Flags: needinfo?(tschuster)
Blocks: 1770772
Flags: needinfo?(tschuster)
Pushed by tschuster@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch
You need to log in before you can comment on or make changes to this bug.