Closed Bug 805527 Opened 7 years ago Closed 7 years ago

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]

Categories

(Core :: XBL, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #675210 - Flags: review?(ehsan)
Attachment #675210 - Flags: review?(ehsan) → review+
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
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.
Keywords: checkin-needed
Keywords: checkin-needed
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.)
https://hg.mozilla.org/mozilla-central/rev/cc081e8dab22
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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)
Depends on: 805787
(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.
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).
Er, sorry, I meant to write bug 805787 for both occurrences of bug 805607 in the previous commment.
You need to log in before you can comment on or make changes to this bug.