1017862 - Remove virtual destructor from BaseProxyHandler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Attachment: Patch

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.

Comment 1

[Jason Orendorff [:jorendorff]]

Comment on attachment 8431150 [details] [diff] [review]
Patch

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

Very nice.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/004d84a6905a

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/94cfbc72d81f for assertion failures: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=40676987&tree=Mozilla-Inbound

Comment 3

[Chris Peterson [:cpeterson]]

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.

Comment 4

[Eric Faust [:efaust]]

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

Comment 5

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06bd71f490fa

yeah, right. Like this'll ever stick. Backed out for blowing up the whole tree.

Comment 6

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b64fd64d1c7

Too many balls in the air. It promise it was backed out for real.

Comment 7

[Eric Faust [:efaust]]

Attachment: Take 2

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.

Comment 8

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d417381e487

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d417381e487