If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 45

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dholbert, Assigned: bz)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8695435 [details]
grep for nsContentUtils within dom/bindings/*.cpp
(Reporter)

Comment 2

2 years ago
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)
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 3

2 years ago
(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

2 years ago
dup of https://bugzilla.mozilla.org/show_bug.cgi?id=1229176 ?
> 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.
(Reporter)

Comment 6

2 years ago
(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.
(Reporter)

Comment 7

2 years ago
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
(Reporter)

Updated

2 years ago
Depends on: 1229176
Summary: MozPaymentProviderBinding.cpp uses nsContentUtils without #including it (which causes build error, depending on unified file-grouping) → bindings codegen still doesn't include nsContentUtils in all cases where necessary
> 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.
Created 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

This fixes the two non-push things.  The push stuff needs non-codegen fixes...
Attachment #8695451 - Flags: review?(continuation)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8695451 - Flags: review?(continuation) → review+
Flags: needinfo?(bzbarsky)

Comment 11

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/61ffbe61bf04
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.
Flags: needinfo?(bkelly)

Comment 13

2 years ago
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.
Flags: needinfo?(bkelly)

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61ffbe61bf04
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Reporter)

Updated

2 years ago
See Also: → bug 1247000
You need to log in before you can comment on or make changes to this bug.