Remove virtual destructor from BaseProxyHandler

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Posted patch PatchSplinter Review
All the C++ proxy handlers are singletons with the same lifetime as the program. There's no need for a desctructor. Add an assert that this is the case.
Attachment #8431150 - Flags: review?(jorendorff)
Comment on attachment 8431150 [details] [diff] [review]
Patch

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

Very nice.
Attachment #8431150 - Flags: review?(jorendorff) → review+
Comment on attachment 8431150 [details] [diff] [review]
Patch

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

::: js/src/jsproxy.h
@@ +122,5 @@
>      void setHasSecurityPolicy(bool aHasPolicy) { mHasSecurityPolicy = aHasPolicy; }
>  
>    public:
>      explicit BaseProxyHandler(const void *family);
> +    ~BaseProxyHandler();

Making BaseProxyHandler's destructor protected is another way to ensure that derived classes do not need virtual destructors.
OK, so the trouble was that we don't run shutdown CC/GCs in non-debug builds, because it's slow, so there was a CC managed Runtime laying around.

Relanded with a relaxed assert.

https://hg.mozilla.org/integration/mozilla-inbound/rev/88fefd05fb64
Flags: needinfo?(efaustbmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/06bd71f490fa

yeah, right. Like this'll ever stick. Backed out for blowing up the whole tree.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b64fd64d1c7

Too many balls in the air. It promise it was backed out for real.
Blocks: 1027425
Posted patch Take 2Splinter Review
So this is basically the same thing, except that we remove the destructor from BaseProxyHandler altogether. This is necessary to make all the proxy handler constructors constexpr, as the compiler wants to be guaranteed of purity, so forbids potential sources of side effects, like non-trivial destructors.

Carrying the r+ for now.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8477809 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0d417381e487
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.