1680223 - Some bindings are using dom StaticPrefs but not including the corresponding header

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), defect, P2)

Description

[Gabriele Svelto [:gsvelto]]

While investigating a b2g/KaiOS build failure I noticed that some of the bindings we generate call StaticPrefs::dom_missing_prop_counters_enabled() without including StaticPrefs_dom.h.

This applies to m-c too, see this call:

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/ImageDocumentBinding.cpp#453

And these are the headers:

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/ImageDocumentBinding.cpp#3-24

That code is being generated from here:

https://searchfox.org/mozilla-central/rev/0d6e8b21569f93a1e1ae8e377ab10f43a6cb12c1/dom/bindings/Codegen.py#14202-14214

I'm not super-familiar with this code so I'll need a little help here. Boris, should we always add StaticPrefs_dom.h to the list of headers we include in the binding when needsMissingPropUseCounters is set in the descriptor? Or should this apply only to the bindings for which missingPropUseCountersForDescriptor gets called?

Note that this isn't causing compilation issues on m-c at the moment because of unified builds.

Comment 1

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

Perhaps peterv has an opinion here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The code at https://searchfox.org/mozilla-central/rev/de782976bf97669f1e8edee59e7a2398154efe06/dom/bindings/Codegen.py#18232-18236 should be generating the right include. Presumably it's failing in this case because the instrumented prop is on HTMLDocument, which ImageDocument inherits from.

Changing that logic accordingly, probably to set descriptorsHaveInstrumentedProps to true if there's descriptor with instrumented props anywhere on its ancestor chain, should help.

Comment 3

[Boris Zbarsky [:bzbarsky]]

And specifically, descriptorsHaveInstrumentedProps should probably be set based on needsMissingPropUseCounters, not based on d.instrumentedProps, if I recall correctly how this works. And should likely be renamed accordingly to descriptorsNeedMissingPropUseCounters. Which would also make it clear that the current condition for UseCounter.h is wrong too...

Comment 4

[[:fabrice] Fabrice Desré]

Gabriele, in the meantime I landed a workaround: https://github.com/kaiostech/gecko-b2g/commit/8dc30447fcbbc1ea6917a511f58fec70a7e89075

Comment 5

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

Attachment: Bug 1680223 - Generate right include for InstrumentedProps;

Comment 6

[Boris Zbarsky [:bzbarsky]]

Doesn't the UseCounter.h condition need adjusting too? That is should descriptorsHaveInstrumentedProps go away completely?

Comment 7

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

For the case that the instrumented prop is from the ancestor, it only uses StaticPref and calls the ancestor's CountMaybeMissingProperty, so we don't need to include UseCounter.h in that case.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20e7fbf94dd7
Generate right include for InstrumentedProps; r=peterv

Comment 9

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/20e7fbf94dd7