Closed Bug 758992 Opened 7 years ago Closed 7 years ago

Make the classes which use the XPCOM nsISupports implementation macros final, to avoid the warning about deleting using a pointer to a base class with virtual functions and no virtual dtor

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: dzbarsky, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(46 files, 3 obsolete files)

32.20 KB, patch
benjamin
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.13 KB, patch
benjamin
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.01 KB, patch
benjamin
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
2.19 KB, patch
smontagu
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
48.10 KB, patch
jduell.mcbugs
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
2.53 KB, patch
benjamin
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
2.78 KB, patch
benjamin
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
4.44 KB, patch
bholley
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
4.42 KB, patch
taras.mozilla
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
10.26 KB, patch
mak
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.19 KB, patch
benjamin
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
2.43 KB, patch
benjamin
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.59 KB, patch
Waldo
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
9.75 KB, patch
bzbarsky
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.06 KB, patch
bholley
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
4.80 KB, patch
hsivonen
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
6.28 KB, patch
joe
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.02 KB, patch
joe
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
12.07 KB, patch
benjamin
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.05 KB, patch
bzbarsky
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
38.77 KB, patch
bzbarsky
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.48 KB, patch
bzbarsky
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
993 bytes, patch
roc
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
2.88 KB, patch
roc
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
77.24 KB, patch
bzbarsky
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
17.70 KB, patch
roc
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
2.72 KB, patch
jrmuizel
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
3.08 KB, patch
jrmuizel
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
964 bytes, patch
roc
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.94 KB, patch
mak
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.55 KB, patch
bholley
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
7.92 KB, patch
jrmuizel
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
844 bytes, patch
Waldo
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.61 KB, patch
jrmuizel
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
3.44 KB, patch
jrmuizel
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.79 KB, patch
jrmuizel
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
10.41 KB, patch
mak
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
2.88 KB, patch
dcamp
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.16 KB, patch
jrmuizel
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
5.71 KB, patch
bzbarsky
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
2.92 KB, patch
roc
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1021 bytes, patch
bzbarsky
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1000 bytes, patch
bzbarsky
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
2.25 KB, patch
mak
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
1.91 KB, patch
mak
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
6.43 KB, patch
bzbarsky
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
Attached patch Patch (obsolete) — Splinter Review
This warning generates ridiculous amount of spam and isn't really useful.
Attachment #627594 - Flags: review?(ted.mielczarek)
Comment on attachment 627594 [details] [diff] [review]
Patch

Review of attachment 627594 [details] [diff] [review]:
-----------------------------------------------------------------

Punting to njn or espindola, who care about warning flags and clang. Whoever reviews it first wins!
Attachment #627594 - Flags: review?(ted.mielczarek)
Attachment #627594 - Flags: review?(respindola)
Attachment #627594 - Flags: review?(n.nethercote)
Ted(In reply to Ted Mielczarek [:ted] from comment #1)
> Comment on attachment 627594 [details] [diff] [review]
> Patch
> 
> Review of attachment 627594 [details] [diff] [review]:

Recent GCCs also support these warnings, with the same arguments syntax. I think it would be better to set it for all GCC-like compilers using MOZ_CXX_SUPPORTS_WARNING, like in other similar cases.
(In reply to Jacek Caban from comment #2)
> Recent GCCs also support these warnings, with the same arguments syntax. I
> think it would be better to set it for all GCC-like compilers using
> MOZ_CXX_SUPPORTS_WARNING, like in other similar cases.

Nevetmind that, I confused that with -Wno-delete-non-virtual-dtor, sorry about that. Anyway, with MOZ_CXX_SUPPORTS_WARNING we wouldn't need to hardcode the logic about which compiler supports which exact flags.
What do you mean it isn't really useful? I thought it was a best practice to make destructors virtual because if you call delete with a non-virtual dtor, the dtor for the type of the pointer gets called, not necessarily the type of the underlying object. I could be wrong: my C++ are far from expert level. FWIW http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7 seems to agree with me. Furthermore, the Clang warning seems to enforce the recommended behavior according to that FAQ.

While there may be many places in the code where not having virtual dtors is OK, you are requesting a global change. This could silence legitimate warnings and lead to memory leaks, etc.

Am I missing something obvious?
Comment on attachment 627594 [details] [diff] [review]
Patch

Same opinion as Gregory Szorc. This warning already found real bugs for me. We were deleting an interface if I remember correctly.

In most cases, the simplest fix is to mark a class final, which also makes it more likely that the compiler will be able to remove virtual calls.
Attachment #627594 - Flags: review?(respindola) → review-
Comment on attachment 627594 [details] [diff] [review]
Patch

I don't have strong opinions on clang-only flags.
Attachment #627594 - Flags: review-
Comment on attachment 627594 [details] [diff] [review]
Patch

Ugh, I mangled the review flags.  Sorry.
Attachment #627594 - Flags: review?(n.nethercote) → review-
The right way to fix this is to mark the classes which generate this type of warning as MOZ_FINAL.
Assignee: dzbarsky → ehsan
Summary: Add -Wno-delete-virtual-dtor to Clang compiler flags → Make the classes which use the XPCOM nsISupports implementation macros final, to avoid the warning about deleting using a pointer to a base class with virtual functions and no virtual dtor
Whiteboard: [build_warning]
Attached patch XPCOM partsSplinter Review
Attachment #627594 - Attachment is obsolete: true
Attachment #630370 - Flags: review?(benjamin)
Whiteboard: [build_warning] → [build_warning][leave open]
Attached patch chrome/ partsSplinter Review
Attachment #630408 - Flags: review?(benjamin)
Attached patch libpref partsSplinter Review
Attachment #630412 - Flags: review?(benjamin)
Attached patch intl/ partsSplinter Review
Attachment #630415 - Flags: review?
Attachment #630415 - Flags: review? → review?(smontagu)
Attached patch netwerk partsSplinter Review
Attachment #630427 - Flags: review?(bzbarsky)
With the patches so far: https://tbpl.mozilla.org/?tree=Try&rev=979b9939f26b
Comment on attachment 630370 [details] [diff] [review]
XPCOM parts

Is MOZ_FINAL being enforced by tinderboxes? I think this is ugly but potentially useful, but don't think we should add these aannotations unless they are being enforced.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> Comment on attachment 630370 [details] [diff] [review]
> XPCOM parts
> 
> Is MOZ_FINAL being enforced by tinderboxes? I think this is ugly but
> potentially useful, but don't think we should add these aannotations unless
> they are being enforced.

They are currently enforced by MSVC (where MOZ_FINAL will get converted to "sealed"), and they will be enforced by clang as well as soon as we get them up (Real Soon Now).  For gcc on Linux, this will be enforced when we switch to gcc 4.7.

In the mean time, this work cleans up the thousands of warnings that people see in their local builds, and will help them notice real bugs where this warning is not issued because of the way we implement XPCOM classes (I've already noticed a couple of these locations -- will file bugs on them shortly).
Comment on attachment 630427 [details] [diff] [review]
netwerk parts

Going to punt this one to jduell.
Attachment #630427 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
Attachment #630370 - Flags: review?(benjamin) → review+
Attachment #630408 - Flags: review?(benjamin) → review+
Attachment #630412 - Flags: review?(benjamin) → review+
Attachment #630370 - Flags: checkin+
Attachment #630408 - Flags: checkin+
Attachment #630412 - Flags: checkin+
Blocks: buildwarning
Whiteboard: [build_warning][leave open] → [leave open]
Component: Build Config → General
QA Contact: build-config → general
Version: unspecified → Trunk
Attachment #630415 - Flags: review?(smontagu) → review+
Attached patch More xpcom partsSplinter Review
Missed these in the previous patch.
Attachment #632516 - Flags: review?(benjamin)
Attachment #632517 - Flags: review?(benjamin)
Attached patch xpconnect partsSplinter Review
Attachment #632521 - Flags: review?(bobbyholley+bmo)
Attached patch libjar partsSplinter Review
Attachment #632522 - Flags: review?(mh+mozilla)
Attached patch storage partsSplinter Review
Attachment #632525 - Flags: review?(mak77)
Attachment #632527 - Flags: review?(benjamin)
Attached patch rdf(!!!) partsSplinter Review
I never thought I'd touch the rdf directory.  <sigh>
Attachment #632529 - Flags: review?(benjamin)
Attached patch jsd partsSplinter Review
Attachment #632530 - Flags: review?(jwalden+bmo)
Attached patch uriloader partsSplinter Review
Attachment #632531 - Flags: review?(bzbarsky)
Attached patch caps partsSplinter Review
Attachment #632533 - Flags: review?(bobbyholley+bmo)
Comment on attachment 632531 [details] [diff] [review]
uriloader parts

r=me
Attachment #632531 - Flags: review?(bzbarsky) → review+
Attached patch parser partsSplinter Review
Attachment #632535 - Flags: review?(hsivonen)
Attached patch gfx partsSplinter Review
Attachment #632539 - Flags: review?(joe)
Attached patch imagelib partsSplinter Review
Attachment #632540 - Flags: review?(joe)
Attachment #630427 - Flags: review?(jduell.mcbugs) → review+
Attachment #632522 - Flags: review?(mh+mozilla) → review?(taras.mozilla)
Attachment #632535 - Flags: review?(hsivonen) → review+
Attachment #632521 - Flags: review?(bobbyholley+bmo) → review+
Attachment #632533 - Flags: review?(bobbyholley+bmo) → review+
Attached patch widget/windows part (obsolete) — Splinter Review
widget/windows parts found by mingw compilation. Other than just marking classes as final, required changing nsDataObjCollection destructor to be virtual.
Attachment #632606 - Flags: review?(benjamin)
Attachment #632539 - Flags: review?(joe) → review+
Attachment #632540 - Flags: review?(joe) → review+
Attachment #632522 - Flags: review?(taras.mozilla) → review+
Attachment #632530 - Flags: review?(jwalden+bmo) → review+
Attachment #632525 - Flags: review?(mak77) → review+
updated version
Attachment #632606 - Attachment is obsolete: true
Attachment #632606 - Flags: review?(benjamin)
Attachment #633074 - Flags: review?
Attachment #633074 - Flags: review? → review?(benjamin)
Attachment #632516 - Flags: review?(benjamin) → review+
Attachment #632517 - Flags: review?(benjamin) → review+
Attachment #632527 - Flags: review?(benjamin) → review+
Attachment #632529 - Flags: review?(benjamin) → review+
Attachment #633074 - Flags: review?(benjamin) → review+
Attachment #633358 - Flags: review?(bzbarsky)
Attached patch dom partsSplinter Review
Attachment #633373 - Flags: review?(bzbarsky)
Comment on attachment 633358 [details] [diff] [review]
More uriloader parts

r=me
Attachment #633358 - Flags: review?(bzbarsky) → review+
Comment on attachment 633373 [details] [diff] [review]
dom parts

1) Making nswebNavigationBaseCommand final makes no sense.

2) For some of the places where you put isupports on all subclasses, wouldn't just putting a virtual destructor on the superclass make more sense?

3) You changed FileInputStreamWrapper and FileOutputStreamWrapper to non-threadsafe refcounting.  Why?  Seems wrong at first glance.

r=me with #1 and #3 fixed and #2 at least considered.
Attachment #633373 - Flags: review?(bzbarsky) → review+
Comment on attachment 633373 [details] [diff] [review]
dom parts

https://hg.mozilla.org/integration/mozilla-inbound/rev/01f3e0c756b0 (with all of the comments addressed)
Attachment #633373 - Flags: checkin+
Attached patch More dom partsSplinter Review
Attachment #634262 - Flags: review?(bzbarsky)
Attached patch view partsSplinter Review
Attachment #634263 - Flags: review?(roc)
Attached patch widget partsSplinter Review
Attachment #634265 - Flags: review?(roc)
Attached patch content partsSplinter Review
Note that I intentionally didn't mark the nsBindingManager as final because of http://llvm.org/bugs/show_bug.cgi?id=13142.
Attachment #634280 - Flags: review?(bzbarsky)
Attached patch layout parts (obsolete) — Splinter Review
Attachment #634281 - Flags: review?(roc)
Attached patch layout partsSplinter Review
GenericTableRule does have derived classes...
Attachment #634281 - Attachment is obsolete: true
Attachment #634281 - Flags: review?(roc)
Attachment #634284 - Flags: review?(roc)
Comment on attachment 634280 [details] [diff] [review]
content parts

nsXULElement.cpp is including Attributes.h twice.  No need for that.

r=me modulo that nit.
Attachment #634280 - Flags: review?(bzbarsky) → review+
Comment on attachment 634262 [details] [diff] [review]
More dom parts

r=me
Attachment #634262 - Flags: review?(bzbarsky) → review+
Comment on attachment 634280 [details] [diff] [review]
content parts

Landed with the nit addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea80e3155b6
Attachment #634280 - Flags: checkin+
Attached patch browser partsSplinter Review
Attachment #634729 - Flags: review?(benjamin)
Attached patch toolkit partsSplinter Review
Attachment #634730 - Flags: review?(benjamin)
Attachment #634731 - Flags: review?(roc)
Attachment #634733 - Flags: review?(mak77)
Attachment #634735 - Flags: review?(bobbyholley+bmo)
Attached patch More xpcom partsSplinter Review
Attachment #634739 - Flags: review?(benjamin)
Attached patch more jsd partsSplinter Review
Attachment #634740 - Flags: review?(jwalden+bmo)
Attachment #634741 - Flags: review?(benjamin)
Attached patch xre partsSplinter Review
Attachment #634742 - Flags: review?(benjamin)
Attachment #634743 - Flags: review?(benjamin)
Attached patch places partsSplinter Review
Attachment #634744 - Flags: review?(mak77)
Attachment #634745 - Flags: review?(dcamp)
Attachment #634735 - Flags: review?(bobbyholley+bmo) → review+
Attachment #634745 - Flags: review?(dcamp) → review+
Attachment #634740 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 634739 [details] [diff] [review]
More xpcom parts

Drive by r+. There's some gonk stuff that snuck in here.
Attachment #634739 - Flags: review?(benjamin) → review+
Comment on attachment 634729 [details] [diff] [review]
browser parts

More drive by
Attachment #634729 - Flags: review?(benjamin) → review+
Attachment #634742 - Flags: review?(benjamin) → review+
Attachment #634741 - Flags: review?(benjamin) → review+
Attachment #634730 - Flags: review?(benjamin) → review+
Attachment #634743 - Flags: review?(benjamin) → review+
Attachment #634730 - Flags: review+ → checkin+
Attachment #634744 - Flags: review?(mak77) → review+
Attachment #634733 - Flags: review?(mak77) → review+
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2e6a50906a to fix a bustage on Windows because class Spinner derives from AsyncStatementSpinner on Windows.
Attached patch embedding partsSplinter Review
Attachment #640901 - Flags: review?(jmuizelaar)
Attached patch docshell partsSplinter Review
Attachment #640902 - Flags: review?(bzbarsky)
Attachment #640903 - Flags: review?(bzbarsky)
Attachment #640903 - Flags: review?(bzbarsky) → review?(roc)
Attachment #640904 - Flags: review?(bzbarsky)
Attached patch More dom partsSplinter Review
Attachment #640905 - Attachment is patch: true
Attachment #640905 - Flags: review?(bzbarsky)
Attachment #640901 - Flags: review?(jmuizelaar) → review+
Attachment #640934 - Flags: review?(sdwilsh)
Attachment #640935 - Flags: review?(sdwilsh)
Attachment #640936 - Flags: review?(benjamin)
Comment on attachment 640934 [details] [diff] [review]
extensions/cookie

Throwing this at Marco!
Attachment #640934 - Flags: review?(sdwilsh) → review?(mak77)
Attachment #640935 - Flags: review?(sdwilsh) → review?(mak77)
Attachment #640936 - Flags: review?(benjamin) → review?(bzbarsky)
Attachment #640934 - Flags: review?(mak77) → review+
Attachment #640935 - Flags: review?(mak77) → review+
Comment on attachment 640902 [details] [diff] [review]
docshell parts

r=me, and a blanket r=me on any changes to this bug that just look like this.  Please do ask for review if the change involves adding actual virtual destructors or doing anything more complicated, though, ok?
Attachment #640902 - Flags: review?(bzbarsky) → review+
Attachment #640904 - Flags: review?(bzbarsky) → review+
Attachment #640905 - Flags: review?(bzbarsky) → review+
Attachment #640936 - Flags: review?(bzbarsky) → review+
(In reply to comment #124)
> r=me, and a blanket r=me on any changes to this bug that just look like this. 
> Please do ask for review if the change involves adding actual virtual
> destructors or doing anything more complicated, though, ok?

Absolutely!
This is fixed now as far as I can tell!
Whiteboard: [leave open]
Target Milestone: --- → mozilla18
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.