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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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...

Comment 1

[Cameron McCormack (:heycam)]

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.

Comment 2

[Cameron McCormack (:heycam)]

Filed bug 1175033 for supporting that at some point.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, so bug 968923 is not going to add something I can just call from C++?  Alright, I guess manual telemetry it is.

Comment 4

[Philip Jägenstedt [:foolip]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 6

[Philip Jägenstedt [:foolip]]

(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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Add telemetry for an Attr with a non-lowercase name that was created from an HTML document being added to a non-HTML element

Nathan, do the telemetry parts of this make sense?  Is there a better way to do them?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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....

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

XHR uses kind: boolean to measure whether it is sync or async.

But yeah, perhaps your setup is better in this case.

Comment 11

[Nathan Froyd [:froydnj]]

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?

Comment 12

[Philip Jägenstedt [:foolip]]

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)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba33a86733d4

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba33a86733d4

Comment 15

[Arkadiusz Michalski (Spirit)]

Philip counter 845 is working properly? I ask because for 844 currently we see some data, but for 845 still not.

Comment 16

[Philip Jägenstedt [:foolip]]

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.