Closed Bug 1175031 Opened 5 years ago Closed 5 years ago

Add telemetry for an Attr with a non-lowercase name being added to a non-HTML element

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

Specifically:

1)  Created via a createAttribute call on an HTML document.
2)  The name contains non-lowercase ASCII chars.
3)  Added to a non-HTML element via setAttributeNode or equivalent via the
    NamedNodeMap.

Need to figure out whether I should just wait for bug 968923 here or do something by hand...
That sounds more complicated than the built-in use counters of bug 968923 could handle.  We should probably support "custom" use counters and not force people to write Telemetry histograms and get the page/document counters right for that.
Filed bug 1175033 for supporting that at some point.
Ah, so bug 968923 is not going to add something I can just call from C++?  Alright, I guess manual telemetry it is.
Continuing from https://bugzilla.mozilla.org/show_bug.cgi?id=1165851#c57

I've looked into measuring this case in Blink, and it would be very simply to just measure the cases where createAttribute is called with a non-lowercase name in HTML documents, which would be the highest limit on the risk. Blink doesn't modify case in setAttributeNode and in principle every case where attr.name would be different is observable.

Does measuring only this case seem useful? There is a risk that usage is still too high and that it'll delay this matter by a few more months.

I've taken a look at httparchive data where createAttribute is called with a string literal, and it sure looks like non-lowercase strings are in the minority, about 2% of the total cases. 75% of those cases were "frameBorder", "oldEvent" or "selectId" and in all of these cases it looks like the assumption is that case doesn't matter and that the code would work as intended with lowercases. I didn't dig too deeply, though. I can't see a single example where it seems like a case-sensitive SVG attribute or similar is intended.
> and it would be very simply to just measure the cases where createAttribute is called
> with a non-lowercase name in HTML documents

That's not terribly useful on its own, because we know this is happening.  That's what bug 1165851 was about.

The real thing that needs to be counted is cases where setAttributeNode happens on a non-HTML element in an HTML document with an Attr whose name is not all-lowercase.  That would be an upper bound on the breakage issues.  To make the bound more exact, we would want to know whether the Attr was created via createAttribute or createAttributeNS.

> Does measuring only this case seem useful?

I don't think so, because I expect a reasonable fraction of pages do this given the bug reports we got....

> I can't see a single example where it seems like a case-sensitive SVG attribute or
> similar is intended.

Right, that's what we really care about.
(In reply to Boris Zbarsky [:bz] from comment #5)
> > and it would be very simply to just measure the cases where createAttribute is called
> > with a non-lowercase name in HTML documents
> 
> That's not terribly useful on its own, because we know this is happening. 
> That's what bug 1165851 was about.

Yeah, but I suspect that even usage of this isn't terribly common, unless what I saw in the httparchive data was very non-representative. I'll measure it too.

> The real thing that needs to be counted is cases where setAttributeNode
> happens on a non-HTML element in an HTML document with an Attr whose name is
> not all-lowercase.  That would be an upper bound on the breakage issues.  To
> make the bound more exact, we would want to know whether the Attr was
> created via createAttribute or createAttributeNS.

OK, this is easy, no tracking required. I'll try to land this:
https://codereview.chromium.org/1195583002

I wouldn't worry about createAttributeNS at all given the usage:
https://www.chromestatus.com/metrics/feature/timeline/popularity/112
Nathan, do the telemetry parts of this make sense?  Is there a better way to do them?
Attachment #8624823 - Flags: review?(nfroyd)
Attachment #8624823 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Blocks: 1176313
Should we count all !aAttr.IsNSAware() SetNamedItemInternal calls on HTMLDocuments and
pass false when element is HTMLElement, and otherwise true? Or something similar to that, so that
we know how much the API is used in general and how often used with !htmlelement.
So that depends on what we're trying to measure.

What I _think_ my patch would measure is what fraction of telemetry pings correspond to users who have seen at least one call to SetNamedItemInternal that matches our criteria.  That is, some measure of what fraction of users might conceivably encounter bustage if we lowercase in createAttribute.

If we want to know what fraction of SetNamedItemInternal calls fall into the failure case, then I suspect we need a non-boolean telemetry thing here and instead use counters for both cases or something.  But I'm not sure, given our telemetry APIs....
XHR uses kind: boolean to measure whether it is sync or async.

But yeah, perhaps your setup is better in this case.
Attachment #8624823 - Flags: review?(bugs) → review+
Comment on attachment 8624823 [details] [diff] [review]
Add telemetry for an Attr with a non-lowercase name that was created from an HTML document being added to a non-HTML element

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

Assuming you just care about whether it has happened, and not what proportion of pages have this behavior, this works for me.  Would you want to use use counters for this kind of thing, so you could get page-level breakdowns?
Attachment #8624823 - Flags: review?(nfroyd) → review+
https://codereview.chromium.org/1195583002 has landed and will be in Chrome 45, which will likely reach stable in August:
https://www.chromestatus.com/metrics/feature/timeline/popularity/844 (HTMLDocumentCreateAttributeNameNotLowercase)
https://www.chromestatus.com/metrics/feature/timeline/popularity/845 (NonHTMLElementSetAttributeNodeFromHTMLDocumentNameNotLowercase)
https://hg.mozilla.org/mozilla-central/rev/ba33a86733d4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Philip counter 845 is working properly? I ask because for 844 currently we see some data, but for 845 still not.
Flags: needinfo?(philipj)
I think it's working, I can hit it in a debug build, and there's one hit on July 20. It's just that usage is very low, how low we'll know when it reaches stable.
Flags: needinfo?(philipj)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.