Closed Bug 527590 Opened 11 years ago Closed 3 years ago

Error on hidden virtual methods

Categories

(Firefox Build System :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: cjones, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

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.
I bet a patch would get review stat.
It's a one-liner, I'm trying it out to see what breaks ;).
Especially since if you actually mean it, `using` will suppress the warning/error. You just get to fix lots of cases in our tree ;-)
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*)’
Attached patch configure.in patch (obsolete) — Splinter Review
-Werror is pretty trivially, looks like all the "fun" will come from fixing the current violators.
(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.
Attached patch full enchilada (obsolete) — Splinter Review
Less pain than I anticipated.  Passing through try as we speak.

Since this patch is fully semantics-preserving, no module r? is needed, right?
Attachment #411321 - Attachment is obsolete: true
Attachment #411349 - Flags: review?(benjamin)
Comment on attachment 411349 [details] [diff] [review]
full enchilada

I think the comments are unnecessary: `using baseclass::Foo` is pretty self-explanatory.
Attachment #411349 - Flags: review?(benjamin) → review-
Attachment #411349 - Attachment is obsolete: true
Attachment #411461 - Flags: review?(benjamin)
Comment on attachment 411461 [details] [diff] [review]
v2, stripped comments

rs=me
Attachment #411461 - Flags: review?(benjamin) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/b68f602ce949
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
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?
(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.
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...
Except that's it's not reordering selectedItem, only getSelectedItem...
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).
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago3 years ago
Resolution: --- → WORKSFORME
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.