1220936 - Consider using nsContentUtils::GetWindowID() when flushing ConsoleReportCollectors

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently the ConsoleReportCollector is flushed to a specific nsIDocument in either the channel or in docshell.  An alternative, would be to use nsContentUtils::GetWindowID() instead.  This method uses the load group to find the associated window instead.

The advantage of the load group approach is it will report the message to the correct console even if the network request was initiated from a worker without a loading document.

Comment 1

[Tom Tung [:tt, :ttung]]

I would like to work on this.

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: init patch.

Sorry about late. I was working on storage API.

Initial patch for this bug.
I listed my changes below inline:
- Add "InnerWindowID" into ReportToConsole() and ReportToConsoleNonLocalized() to allow using window ID directly when we don't have a document.
- Rewrite FlushReportsByWindowID and FlushConsoleReports() since they are simlar.


Next: thinking about maybe create a method that flush report into loadGroup since both document(GetDocumentLoadGeoup()) and worker(GetLoadGroup) can get loadgroup easily

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: Flush console reports to innerWindowID rather than nsIDocument.

Hi Ben,

In this patch, I intend to flush report based on window ID rather nsIDocument. However, I'm not so sure about the approach is suitable. Could you help me to review this patch? Thanks!

To do this, I list the things I did as following:
- Rewrite FlushConsoleReports(doc) and change its name to FlushReportsToConsole(id) since I think it's better to provide one way to flush rather than two at the same time, and I want to avoid being ambiguous with FlushConsoleReports(nsIConsoleReportCollector* aCollector) (e.g. FlushConsoleReports(0)).

- Remove FlushReportByWindowid() since it's redundant.

- Split the ReportToConsoleNonLocalized() into two pieces and name later one as ReportToConsoleByWindowID() to flush console report via window ID.

- Split the GetInnerWindowID() into two pieces since sometimes we have a loadGroup instead of a request.

- Get inner window ID from loadGroup/nsIRequest rather than document at first when calling FlushReportsToConsole(). If we don't have a loadgroup/request, it will follow the original logic to get ID from the document.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8830652 [details] [diff] [review]
Flush console reports to innerWindowID rather than nsIDocument.

Review of attachment 8830652 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good.  Thanks!

I think we could just avoid a bit of code duplication with a few extra convenience methods.  r=me with that change.

::: dom/console/ConsoleReportCollector.h
@@ +27,5 @@
>                     const nsTArray<nsString>& aStringParams) override;
>  
>    void
> +  FlushReportsToConsole(uint64_t aInnerWindowID,
> +                        ReportAction aAction = ReportAction::Forget) override;

Can you please add a FlushReportsToConsole() that takes nsILoadGroup* and does the nsContentUtils::GetInnerWindowID() automatically?  It seems we duplicate that code around a lot here.

Also perhaps there should be a FlushReportsToConsole() that takes nsIDocument* and does the `doc ? doc->InnerWindowID() : 0` logic.

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1220936-v2-Flush console report to innerWindowID by using nsIDocument and nsILoadGroup.

Addressed comment, but I'm a bit afraid I did somethings wrong here. 

I list the changes in this patch as follow:
- Create two functions (FlushConsoleReports(doc) and FlushConsoleReports(loadGroup)) to handle the redundant logic. I avoid naming these two functions from FlushReportToConsole because it may be ambiguous when FlushReportToConsole(0). 

- Limit FlushConsoleReports(doc) be called in MainThread. However, FlushConsoleReports(loadGroup) can be called in any thread since it supposes to be called anywhere even workers.

Since I did many changes in this patch, could you help me to review it one more time, Ben? I'll provide the inter-diff patch later. Thanks!

Comment 6

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-P1-2.patch

inter-diff patch

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832823 [details] [diff] [review]
Bug-1220936-v2-Flush console report to innerWindowID by using nsIDocument and nsILoadGroup.

Thanks!

Comment 8

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=813360cce8f5

Comment 9

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e364bc537f3

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: [Final]Bug-1220936-Flush console report to innerWindowID by using nsIDocument and nsILoadGroup. r=bkelly.

(In reply to Ben Kelly [:bkelly] from comment #7)
> Comment on attachment 8832823 [details] [diff] [review]
> Bug-1220936-v2-Flush console report to innerWindowID by using nsIDocument
> and nsILoadGroup.
> 
> Thanks!

Thanks for the review!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4ed23eb2f3
Flush console report to innerWindowID by using nsIDocument and nsILoadGroup. r=bkelly.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/0d4ed23eb2f3