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.

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

(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

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.

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?

(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.

(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.

Jocelyn, can you try the attached patch?

(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

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

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