527590 - Error on hidden virtual methods

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For example

class Base {
public:
    virtual int Foo() { return 0; }
};
class Derived : public Base {
public:
    virtual int Foo(int) { return 1; }
};

produces the warning (because of -Woverloaded-virtual)

hidden.cc:3: error: ‘virtual int Base::Foo()’ was hidden
hidden.cc:8: error:   by ‘virtual int Derived::Foo(int)’

but empirically, I've only seen this lead to bugs.  Has anyone found valid uses?

If not, I suggest that we just compile with -Werror=overloaded-virtual.

Comment 1

[Shawn Wilsher :sdwilsh]

I bet a patch would get review stat.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It's a one-liner, I'm trying it out to see what breaks ;).

Comment 3

[Benjamin Smedberg]

Especially since if you actually mean it, `using` will suppress the warning/error. You just get to fix lots of cases in our tree ;-)

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Here's an error that seems pretty serious:

In file included from /home/cjones/mozilla/mozilla-central/content/html/document/src/nsHTMLDocument.h:42,
                 from /home/cjones/mozilla/mozilla-central/dom/base/nsDOMClassInfo.cpp:138:
../../dist/include/nsINode.h:874: error: ‘virtual PRUint32 nsINode::GetScriptTypeID() const’ was hidden
/home/cjones/mozilla/mozilla-central/content/base/src/nsDocument.h:907: error:   by ‘virtual nsresult nsDocument::GetScriptTypeID(PRUint32*)’

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: configure.in patch

-Werror is pretty trivially, looks like all the "fun" will come from fixing the current violators.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> Here's an error that seems pretty serious:
> 
> In file included from
> /home/cjones/mozilla/mozilla-central/content/html/document/src/nsHTMLDocument.h:42,
>                  from
> /home/cjones/mozilla/mozilla-central/dom/base/nsDOMClassInfo.cpp:138:
> ../../dist/include/nsINode.h:874: error: ‘virtual PRUint32
> nsINode::GetScriptTypeID() const’ was hidden
> /home/cjones/mozilla/mozilla-central/content/base/src/nsDocument.h:907: error: 
>  by ‘virtual nsresult nsDocument::GetScriptTypeID(PRUint32*)’

On second blush, this one looks pretty benign, just dumb.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: full enchilada

Less pain than I anticipated.  Passing through try as we speak.

Since this patch is fully semantics-preserving, no module r? is needed, right?

Comment 8

[Benjamin Smedberg]

Comment on attachment 411349 [details] [diff] [review]
full enchilada

I think the comments are unnecessary: `using baseclass::Foo` is pretty self-explanatory.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v2, stripped comments

Comment 10

[Benjamin Smedberg]

Comment on attachment 411461 [details] [diff] [review]
v2, stripped comments

rs=me

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/mozilla-central/rev/b68f602ce949

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> Pushed http://hg.mozilla.org/mozilla-central/rev/b68f602ce949
<3

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Backed out.  Apparently this change may have altered a vtable on windows and caused crashiness.  If so, this patch may need to be flushed down the toilet.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It was causing a crash that went through a call from C++ into JS on the one interface modified by the patch, in particular:

Thread 0 (crashed)
 0  xul.dll!XPCConvert::NativeInterface2JSObject(XPCLazyCallContext &,int *,nsIXPConnectJSObjectHolder * *,nsISupports *,nsID const *,XPCNativeInterface * *,nsWrapperCa
che *,JSObject *,int,int,unsigned int *) [xpcconvert.cpp:a0c008161bd3 : 1116 + 0xc]
    eip = 0x60459194   esp = 0x0012db94   ebp = 0x0012dc04   ebx = 0x06c91350
    esi = 0x0012e0c0   edi = 0x00000000   eax = 0x00000000   ecx = 0x0012dc24
    edx = 0x6aaaa30d   efl = 0x00010246
    Found by: given as instruction pointer in context
 1  xul.dll!XPCConvert::NativeData2JS(XPCLazyCallContext &,int *,void const *,nsXPTType const &,nsID const *,JSObject *,unsigned int *) [xpcconvert.cpp:a0c008161bd3
 : 471 + 0x18]
    eip = 0x60459df8   esp = 0x0012dc0c   ebp = 0x0012dc48   ebx = 0x0012de08
    Found by: call frame info
 2  xul.dll!XPCConvert::NativeData2JS(XPCCallContext &,int *,void const *,nsXPTType const &,nsID const *,JSObject *,unsigned int *) [xpcprivate.h:a0c008161bd3 : 299
3 + 0x16]
    eip = 0x60453b06   esp = 0x0012dc50   ebp = 0x0012dcdc
    Found by: call frame info
 3  xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *,unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [xpcwrappedjsclass.cpp:a0c008161bd3 : 1572 + 0x
22]
    eip = 0x604948d8   esp = 0x0012dd40   ebp = 0x0012df14   ebx = 0x00000000
    Found by: call frame info
 4  xul.dll!nsXPCWrappedJS::CallMethod(unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [xpcwrappedjs.cpp:a0c008161bd3 : 570 + 0x15]
    eip = 0x604912a4   esp = 0x0012df80   ebp = 0x0012e044   ebx = 0x00000004
    Found by: call frame info
 5  xul.dll!PrepareAndDispatch [xptcstubs.cpp:a0c008161bd3 : 114 + 0x14]
    eip = 0x60ae5902   esp = 0x0012df98   ebp = 0x0012e044
    Found by: call frame info
 6  xul.dll!SharedStub [xptcstubs.cpp:a0c008161bd3 : 141 + 0x4]
    eip = 0x60ae5969   esp = 0x0012e04c   ebp = 0x0012e060   ebx = 0x03028a70
    Found by: call frame info
 7  xul.dll!PresShell::GetCurrentItemAndPositionForElement(nsIDOMElement *,nsIContent * *,nsIntPoint &) [nsPresShell.cpp:a0c008161bd3 : 6918 + 0x1e]
    eip = 0x6059468e   esp = 0x0012e068   ebp = 0x0012e060
    Found by: call frame info with scanning

where line 6918 of nsPresShell.cpp is (see http://hg.mozilla.org/mozilla-central/file/a0c008161bd3/layout/base/nsPresShell.cpp#l6918 ) a line that calls through the single interface (nsIDOMXULMultiSelectControlElement) that was modified by this patch.

Smells like either the vtable got changed or there's a build system dependency problem.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In addition to the crash on mochitest 5/5, it also caused a timeout on crashtest, a crash on mochitest-chrome, and many failures on mochitest-a11y.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Thinking on this with a clearer mind, these failures make sense.  This patch was just wrong.  Any suggestions on a "real" fix?  I'd still really like to see this land.

It seems to me that we have two options (i) pull the overridden base class's decl into the derived interface decl, explicitly overriding it; (ii) change the overridden method names.  I'm somewhat uneasy about both since they both require changes to some old and scary-looking interfaces.  But the former change seems "safer" in that it only adds to the interface.  Thoughts?

Comment 17

[timeless]

(i) should be simpler, (ii) would in theory be friendlier to cross reference consumers but requires considerably more effort since you have to invent and agree on new method names.

if you don't mind, i'd rather you use a new bug for whichever solution you select. having a series of dead patches and other baggage in this bug doesn't help people.

Comment 18

[[e-mail]]

I hit the crash in the debugger; the JS callee was trying to return "multiple" as the value of the "selType" property but the C++ caller was trying to call GetSelectedItem(i, getter_AddRefs(node)), which isn't even the one specified in the using statement, so MSVC is doing reordering of overloaded virtual methods, which will kill (i) stone dead...

Comment 19

[[e-mail]]

Except that's it's not reordering selectedItem, only getSelectedItem...

Comment 20

[[e-mail]]

Moving the using to the end of the interface seems to fix the issue, but I haven't tried a full rebuild yet (I only built a test file).

Comment 21

[Gregory Szorc [:gps]]

The current build system adds -Woverloaded-virtual by default. Some vendored projects disable this warning. So I'm assuming this has been fixed in the 8 years since there was activity on this bug.