Closed Bug 1165184 Opened 4 years ago Closed 4 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: yrliou, Unassigned)

References

Details

(Whiteboard: [webbt-api])

Attachments

(3 files)

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.
This is the patch I use to enable bluetooth2.
Attached file 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)));
                                                       ^
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
Flags: needinfo?(ttromey)
(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
Component: Bluetooth → DOM
Product: Firefox OS → Core
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.
Flags: needinfo?(ttromey)
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?
Flags: needinfo?(peterv)
(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.
(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.
Duplicate of this bug: 1166251
(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.
Flags: needinfo?(peterv)
Jocelyn, can you try the attached patch?
Flags: needinfo?(joliu)
(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!
Hi Tom,

The patch also works for me.

Thanks,
Jocelyn
Flags: needinfo?(joliu)
Attachment #8607520 - Flags: review?(peterv)
Making the summary searchable -- I had a really hard time finding this bug.
Summary: [bluetooth2] Build break on nexus5-l → [bluetooth2] Build break on nexus5-l (NodeListBinding.cpp:32:15: error: no matching function for call to 'StrongOrRawPtr')
Blocks: 1167064
Attachment #8607520 - Flags: review?(peterv) → review+
This patch moves code around without modifying it, so I considered a build-only "try" sufficient.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0cf2984f2c59
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.