805527 - nsBindingManager.cpp:433:1613: warning: deleting object of polymorphic class type ‘nsBindingManager’ which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Daniel Holbert [:dholbert]]

content/xbl/src/nsBindingManager.cpp:433:1613: warning: deleting object of polymorphic class type ‘nsBindingManager’ which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]

Marking the class as MOZ_FINAL fixes the warning, so I think that's what we want to do here.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix: mark class as MOZ_FINAL

Comment 2

[Daniel Holbert [:dholbert]]

Actually, this is the only warning that I hit in this directory, w/ g++-4.7 (in both opt & debug builds) -- so with it fixed, I'm pretty sure it's safe to add FAIL_ON_WARNINGS.

Pushed this with that addition to Try, for Mac & Linux, to be sure there aren't outstanding clang warnings or anything:  https://tbpl.mozilla.org/?tree=Try&rev=604c4c1e860d

Comment 3

[Daniel Holbert [:dholbert]]

Yup, I hit some clang warnings (treated as errors) in that try push when I added FAIL_ON_WARNINGS, so I won't bother doing that here.  I filed bug 805555 on the clang warnings.

So: attached patch (which lacks the FAIL_ON_WARNINGS annotation) is checkin-needed.

Comment 4

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc081e8dab22

Comment 5

[(no longer active)]

FWIW, I had intentionally left this out as it breaks older clang builds (see http://llvm.org/bugs/show_bug.cgi?id=13142) but I believe that enough time has passed since this was fixed in clang.)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc081e8dab22

Comment 7

[Jonathan Kew [:jfkthame]]

This seems to have broken the build on OS X 10.7, at least, using the latest version of xcode offered for this system.

Undefined symbols for architecture x86_64:
  "nsIMutationObserver::ContentRemoved(nsIDocument*, nsIContent*, nsIContent*, int, nsIContent*)", referenced from:
      nsNodeUtils::ContentRemoved(nsINode*, nsIContent*, int, nsIContent*) in nsNodeUtils.o
  "nsIMutationObserver::ContentAppended(nsIDocument*, nsIContent*, nsIContent*, int)", referenced from:
      nsNodeUtils::ContentAppended(nsIContent*, nsIContent*, int) in nsNodeUtils.o
  "nsIMutationObserver::ContentInserted(nsIDocument*, nsIContent*, nsIContent*, int)", referenced from:
      nsNodeUtils::ContentInserted(nsINode*, nsIContent*, int) in nsNodeUtils.o
  "nsIMutationObserver::AttributeChanged(nsIDocument*, mozilla::dom::Element*, int, nsIAtom*, int)", referenced from:
      nsNodeUtils::AttributeChanged(mozilla::dom::Element*, int, nsIAtom*, int) in nsNodeUtils.o
  "nsIMutationObserver::AttributeWillChange(nsIDocument*, mozilla::dom::Element*, int, nsIAtom*, int)", referenced from:
      nsNodeUtils::AttributeWillChange(mozilla::dom::Element*, int, nsIAtom*, int) in nsNodeUtils.o
  "nsIMutationObserver::CharacterDataChanged(nsIDocument*, nsIContent*, CharacterDataChangeInfo*)", referenced from:
      nsNodeUtils::CharacterDataChanged(nsIContent*, CharacterDataChangeInfo*) in nsNodeUtils.o
  "nsIMutationObserver::CharacterDataWillChange(nsIDocument*, nsIContent*, CharacterDataChangeInfo*)", referenced from:
      nsNodeUtils::CharacterDataWillChange(nsIContent*, CharacterDataChangeInfo*) in nsNodeUtils.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> FWIW, I had intentionally left this out as it breaks older clang builds (see
> http://llvm.org/bugs/show_bug.cgi?id=13142) but I believe that enough time
> has passed since this was fixed in clang.)

Now that we're relying on clang rather than gcc as our standard compiler on OS X, I don't think 4 months is "enough time" since a fix landed in clang trunk. Unless there's a _really_ compelling reason to break things, it should be possible to build our codebase with the standard toolchain that Apple is shipping on our supported system versions.

Comment 9

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

That said, I think taking bug 805607 is the right way forward (rather than backing this out), since the code that clang miscompiled was a hacky workaround for a mistake I made in bug 232351 (forgetting "public:", which is fixed in bug 805607).

Comment 10

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

Er, sorry, I meant to write bug 805787 for both occurrences of bug 805607 in the previous commment.