1165184 - [bluetooth2] Build break on nexus5-l (NodeListBinding.cpp:32:15: error: no matching function for call to 'StrongOrRawPtr')

Status: RESOLVED FIXED

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

Description

[Jocelyn Liu [:jocelyn] [:joliu]]

We encounter build breaks on nexus5-l when building bluetooth2 using m-c.
The result of 'git bisect' is the commit of Bug 1152033, I can build bluetooth2 using m-c after revert this commit.

I will attach APIv2 enable patch and the build error log here.

Comment 1

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Dev] Activate Bluetooth v2 with LE support

This is the patch I use to enable bluetooth2.

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: api-v2-buildfail.txt

This is the build log.

Error is:

UnifiedBindings14.o
In file included from UnifiedBindings14.cpp:2:0:
NodeListBinding.cpp: In function 'bool mozilla::dom::NodeListBinding::item(JSContext*, JS::Handle<JSObject*>, nsINodeList*, const JSJitMethodCallArgs&)':
NodeListBinding.cpp:32:55: error: no matching function for call to 'StrongOrRawPtr(nsIContent*)'
   auto result(StrongOrRawPtr<nsINode>(self->Item(arg0)));
                                                       ^

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Tom,

We use a build flag to manually switch to bluetooth APIv2 for development.
(v1 will be deprecated on m-c about mid of June.)
And we recently found that we cannot build our api2 on nexus-5-l anymore with your patch in Bug1152033 applied, but flame-kk is still fine.

I have no idea why we will encounter the error mentioned in Comment 2 will occur with your patch applied.
And we cannot formally switch to v2 on m-c if this will break nexus-5-l build.
Any thoughts why this happened?

Please let me know if you need more information from me.

Thanks,
Jocelyn

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #3)
> Hi Tom,
> 
> We use a build flag to manually switch to bluetooth APIv2 for development.
> (v1 will be deprecated on m-c about mid of June.)
> And we recently found that we cannot build our api2 on nexus-5-l anymore
> with your patch in Bug1152033 applied, but flame-kk is still fine.
> 
> I have no idea why we will encounter the error mentioned in Comment 2 will
> occur with your patch applied.
Sorry, typo.
I have no idea why we will encounter the error mentioned in Comment 2 with your patch applied.
> And we cannot formally switch to v2 on m-c if this will break nexus-5-l
> build.
> Any thoughts why this happened?
> 
> Please let me know if you need more information from me.
> 
> Thanks,
> Jocelyn

Comment 5

[Tom Tromey :tromey]

I'm wondering if somehow the visible #includes changed between builds.

The failing code in NodeListBinding.cpp:

  auto result(StrongOrRawPtr<nsINode>(self->Item(arg0)));

self->Item(arg0) has type nsIContent*, but this is casting to nsINode*.
However, NodeListBinding.cpp does not include nsIContent.h.

So one way this could fail is if previously somehow nsIContent.h was
included, but is not included for some reason (and here let me wave
my hands a lot, since I don't know anything about the unified build
machinery).

This theory relies on GCC not mentioning that nsIContent is an incomplete
type at the point of the cast.  Which seems a bit far-fetched.


Now, if nsIContent is complete at this point, then I am really puzzled.
The error from the template instantiation that I would expect to apply
says:

../../dist/include/mozilla/dom/BindingUtils.h:3219:1: note:   template argument deduction/substitution failed:
In file included from UnifiedBindings14.cpp:2:0:
NodeListBinding.cpp:32:55: note:   cannot convert 'self->nsINodeList::Item(arg0)' (type 'nsIContent*') to type 'nsINode*'
   auto result(StrongOrRawPtr<nsINode>(self->Item(arg0)));

... so if it is seeing the full definition of nsIContent, then it doesn't
seem possible for this error to arise.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, presumably what happened here is that moving things across unified file boundaries moved NodeListBinding to a unified file that doesn't include nsIContent.

Now normally the right way to fix this would be to have nsINodeList.h include nsIContent.h, but in this case that won't work because nsINode.h includes nsINodeList.h (because it declares a subclass of nsINodeList).

So I think we have the following options (at least; maybe there are others):

1)  Include nsIContent.h in BindingUtils.h, so all bindings .cpp files including this one
    see it.
2)  Include nsIContent.h in the binding cpp for NodeList explicitly, via a codegen change.
3)  Move nsChildContentList out of nsINode.h into a separate header, move the include of
    nsINodeList.h to that header, have nsINodeList.h include nsIContent.h, move the
    nsINode::Slots constructor (which needs the definition of nsChildContentList) to
    either be out of line or in a separate nsINode-inlines.h.

I think in theory #3 is the theoretically right solution, but #2 is the expedient one...  Peter, any objections?

Comment 7

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #5)

> This theory relies on GCC not mentioning that nsIContent is an incomplete
> type at the point of the cast.  Which seems a bit far-fetched.

GCC indeed doesn't mention the type's incompleteness here.
I filed a GCC bug for this.

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Not doing reviews right now from comment #6)
> Yes, presumably what happened here is that moving things across unified file
> boundaries moved NodeListBinding to a unified file that doesn't include
> nsIContent.
> 
> Now normally the right way to fix this would be to have nsINodeList.h
> include nsIContent.h, but in this case that won't work because nsINode.h
> includes nsINodeList.h (because it declares a subclass of nsINodeList).
> 
> So I think we have the following options (at least; maybe there are others):
> 
> 1)  Include nsIContent.h in BindingUtils.h, so all bindings .cpp files
> including this one
>     see it.
> 2)  Include nsIContent.h in the binding cpp for NodeList explicitly, via a
> codegen change.
> 3)  Move nsChildContentList out of nsINode.h into a separate header, move
> the include of
>     nsINodeList.h to that header, have nsINodeList.h include nsIContent.h,
> move the
>     nsINode::Slots constructor (which needs the definition of
> nsChildContentList) to
>     either be out of line or in a separate nsINode-inlines.h.
> 
> I think in theory #3 is the theoretically right solution, but #2 is the
> expedient one...  Peter, any objections?

I don't know how to revise the codegen, so I only test #1, and I can build bluetooth api2 on nexus-5-l with #1.
So I think the build problem looks like what we think it is here.

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to Not doing reviews right now from comment #6)
> I think in theory #3 is the theoretically right solution, but #2 is the
> expedient one...  Peter, any objections?

I really want us to do 3, so if we do 2 then file a followup.

Comment 11

[Tom Tromey :tromey]

Attachment: move nsChildContentList to its own header

Comment 12

[Tom Tromey :tromey]

Jocelyn, can you try the attached patch?

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Tom Tromey :tromey from comment #11)
> Created attachment 8607520 [details] [diff] [review]
> move nsChildContentList to its own header

I verified this patch. It works for me!

Comment 14

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Tom,

The patch also works for me.

Thanks,
Jocelyn

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Making the summary searchable -- I had a really hard time finding this bug.

Comment 16

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=635a29eab466

Comment 17

[Tom Tromey :tromey]

This patch moves code around without modifying it, so I considered a build-only "try" sufficient.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/0cf2984f2c59

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0cf2984f2c59