Closed Bug 1220936 Opened 4 years ago Closed 3 years ago

Consider using nsContentUtils::GetWindowID() when flushing ConsoleReportCollectors

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bkelly, Assigned: tt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
I would like to work on this.
Assignee: nobody → ttung
Attached patch init patch. (obsolete) — Splinter Review
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
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.
Attachment #8827837 - Attachment is obsolete: true
Attachment #8830652 - Flags: review?(bkelly)
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.
Attachment #8830652 - Flags: review?(bkelly) → review+
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!
Attachment #8830652 - Attachment is obsolete: true
Attachment #8832823 - Flags: review?(bkelly)
Attached patch interdiff-P1-2.patch (obsolete) — Splinter Review
inter-diff patch
Comment on attachment 8832823 [details] [diff] [review]
Bug-1220936-v2-Flush console report to innerWindowID by using nsIDocument and nsILoadGroup.

Thanks!
Attachment #8832823 - Flags: review?(bkelly) → review+
(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!
Attachment #8832823 - Attachment is obsolete: true
Attachment #8832824 - Attachment is obsolete: true
Attachment #8833870 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4ed23eb2f3
Flush console report to innerWindowID by using nsIDocument and nsILoadGroup. r=bkelly.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d4ed23eb2f3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.