1230291 - bindings codegen still doesn't include nsContentUtils in all cases where necessary

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Build error that I hit locally today:
{
 In file included from $OBJDIR/dom/bindings/UnifiedBindings12.cpp:86:
 ./MozPaymentProviderBinding.cpp:1336:31: error: use of undeclared identifier 'nsContentUtils'; did you mean 'nsContentList'?
                               nsContentUtils::ThreadsafeIsCallerChrome() ? &sChromeOnlyNativeProperties : nullptr,
}

(Ignore the "nsContentList" suggestion; that's a red herring.)

The problem here is that MozPaymentProviderBinding.cpp is missing a nsContentUtils.h include.  Depending on how it gets mushed into a unified .cpp file, it may get one "for free" from some other .cpp file -- or it may not. (It didn't in my case.)

I'm not sure why this file in particular is missing that #include; it looks like most (but not all) other $OBJDIR/dom/bindings that use nsContentUtils do have an include for it.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: grep for nsContentUtils within dom/bindings/*.cpp

Comment 2

[Daniel Holbert [:dholbert]]

From a visual skim of the attached grep results, here are the files which have nsContentUtils usages but are missing an #include:
 MozPaymentProviderBinding.cpp
 PushEventBinding.cpp
 PushMessageDataBinding.cpp
 ServiceWorkerRegistrationBinding.cpp
 XPathEvaluatorBinding.cpp

bz, do you know how we decide whether to use this #include in a generated XYZBinding.cpp file? (and what code-generation stuff we might need to fix to make us add this #include for these bindings)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> From a visual skim of the attached grep results

(Sorry, this is actually from a multi-step grep, not from a visual skim. Forgot to update first line of comment, after changing my mind to distrust my initial visual skim.)

Comment 4

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

dup of https://bugzilla.mozilla.org/show_bug.cgi?id=1229176 ?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I'm not sure why this file in particular is missing that #include

Looks like the magic sauce here is a JSImplemented interface with a [Cached] thing on it.  Oh, and it needs to be NoInterfaceObject because otherwise descriptorHasChromeOnly in Codegen.py will generate the include.

We should add a check for clearableCachedAttrs(descriptor) in descriptorHasChromeOnly for the JSImplemented case.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Olli Pettay [:smaug] from comment #4)
> dup of https://bugzilla.mozilla.org/show_bug.cgi?id=1229176 ?

Dup in spirit (in that the summary is basically the same), but not a dupe in the sense that I'm seeing this with up-to-date mozilla-central, and bug 1229176's patch was merged to mozilla-central 2 days ago.

Comment 7

[Daniel Holbert [:dholbert]]

For reference, my hacky grep commands to generate the list in comment 2 were (starting in objdir):

  cd dom/bindings
  grep nsContentUtils:: *.cpp | cut -f1 -d':' | uniq > /tmp/users.txt
  for x in `cat /tmp/users.txt`; do grep nsContentUtils.h $x > /dev/null; echo $? $x; done | grep 1

Comment 8

[Boris Zbarsky [:bzbarsky]]

> dup of https://bugzilla.mozilla.org/show_bug.cgi?id=1229176 ?

No, my tree has that change but still has the problem for MozPaymentProviderBinding.

Looking at the rest of comment 2:

> PushEventBinding.cpp

This is because it uses a Func from nsContentUtils but _also_ overrides its own headerFile, which means that bindings won't try to guess which headers its Funcs live on based on their classnames (since that guess would, for example, be wrong for PushEvent::* funcs).  The right fix here is to stop overriding headerFile or to use a Func from the same header as the actual class, or from some header that is included by the class header.

> PushMessageDataBinding.cpp

Same issue as PushEventBinding.

> ServiceWorkerRegistrationBinding.cpp

Same issue as PushEventBinding (and these are all the same Func too).

> XPathEvaluatorBinding.cpp

This is a codegen bug.  CGClassHasInstanceHook uses nsContentUtils::XPConnect but we don't necessarily include it just because we're going to use it.  Simple enough to fix, luckily.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Correctly include nsContentUtils when generating a manual hasInstance with nsIDOM* stuff or when we have a JS-implemented interface with clearable cached attrs

This fixes the two non-push things.  The push stuff needs non-codegen fixes...

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8695451 [details] [diff] [review]
Correctly include nsContentUtils when generating a manual hasInstance with nsIDOM* stuff or when we have a JS-implemented interface with clearable cached attrs

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

::: dom/bindings/Codegen.py
@@ +1037,5 @@
>                                    d.interface.hasInterfaceObject() and
>                                    NeedsGeneratedHasInstance(d) and
>                                    d.interface.hasInterfacePrototypeObject())
> +        if len(hasInstanceIncludes) > 0:
> +            # We're going to need nsContentUtils too

IMHO the comment is a bit redundant...

@@ +13147,2 @@
>  
>              return (any(isChromeOnly(a) for a in desc.interface.members) or

Boy, this is an ugly block of code. Though it is comprehensible with the comments.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61ffbe61bf04

Comment 12

[Boris Zbarsky [:bzbarsky]]

So as far as the Push stuff, it's used from three places: PushEvent, PushMessageData, ServiceWorkerRegistration.  The corresponding headerFile annotations in Bindings.conf are ServiceWorkerEvents.h, ServiceWorkerEvents.h, and mozilla/dom/ServiceWorkerRegistration.h.

ServiceWorkerEvents.h includes nsContentUtils.h, added in <http://hg.mozilla.org/mozilla-central/rev/ffd18bccd1ce>.  So those are OK.

ServiceWorkerRegistration.h does not explicitly include nsContentUtils.h.  I'm not even sure it's including it indirectly.  So at a guess ServiceWorkerRegistration is bootlegging this header right now.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Of the options in comment 8, I would go with "use a Func from the same header as the actual class" and proxy to nsContentUtils.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61ffbe61bf04