XMLDocument.load warnings/telemetry are useless

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

We're calling WarnOnceAbout on the XMLDocument itself.  In the "normal" load() use case, this won't be a document loaded in a docshell, so won't have a windowid (so warning won't be visible) or get telemetry accumulated for it.

We should warn on the entry doc instead, if we have one.

This still won't help load() calls from JSMs, so we might have no way of knowing whether we have chrome callers into this stuff.  :(
Created attachment 8801188 [details] [diff] [review]
Fix the document.load() warning and telemetry to have a shot at actually being noticed
Attachment #8801188 - Flags: review?(nfroyd)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8801188 [details] [diff] [review]
Fix the document.load() warning and telemetry to have a shot at actually being noticed

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

r=me, I guess, with the below change--assuming I'm not missing something.

::: dom/xml/XMLDocument.cpp
@@ +292,5 @@
>    }
>  
> +  // Reporting a warning on ourselves is rather pointless, because we probably
> +  // have no window id and probably aren't considered a "content document"
> +  // because we're not loaded in a docshell, so won't accumulate telemetry for

Just to be clear, this fix is really only for the telemetry, right?  As I read WarnOnceAbort, there's no discussion of window IDs and content documents there.  So this comment should be amended, along with the commit message?
Attachment #8801188 - Flags: review?(nfroyd) → review+
> Just to be clear, this fix is really only for the telemetry, right? 

No.

> As I read WarnOnceAbort, there's no discussion of window IDs and content documents there.

The window ID part is key for WarnOnceAbout.  Not having one means the console log doesn't show up in web console, because we don't know which web console to show it in.  I'll make the comments clearer.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> > As I read WarnOnceAbort, there's no discussion of window IDs and content documents there.
> 
> The window ID part is key for WarnOnceAbout.  Not having one means the
> console log doesn't show up in web console, because we don't know which web
> console to show it in.  I'll make the comments clearer.

Ah, thanks for the explanation; I didn't look closely enough.

Comment 5

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21ba15fa0c44
Fix the document.load() warning and telemetry to have a shot at actually being noticed.  r=froydnj

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21ba15fa0c44
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

5 months ago
Blocks: 332175
You need to log in before you can comment on or make changes to this bug.