1196383 - Mark the refcount field of JSPrincipals as mutable

Status: ASSIGNED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

There are places that correctly use const JSPrincipals pointers, but currently you can't do much with them because JS_HoldPrincipals and JS_DropPrincipals take non-const pointers.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Mark the refcount field of JSPrincipals as mutable

As usual with const stuff, this snowballed out of control. Marking the refcount field mutable was not a problem, of course. But to make JS_DropPrincipals accept const required releasing principals to be const, which *should* be fine because you're essentially just deleting the dang thing. Sadly, JSPrincipals are the SM-visible incarnation of an nsJSPrincipals, which uses XPCOM refcounting, which is non-const. And I don't want to change that at the moment, so I added an unsafe const_cast. Curious how you feel about it.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8650031 [details] [diff] [review]
Mark the refcount field of JSPrincipals as mutable

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

I don't really see a huge amount of value here - principals are already immutable (modulo document.domain), so making them const doesn't buy us very much. And all this weird const casting and undefined behavior makes me nervous. It feels strange to me to be able to delete something that's const, but I could be convinced of that if it was accepted elsewhere and bought us something useful.

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Bobby Holley (:bholley) from comment #2)
> It feels strange to me to be able to delete something that's const,
> but I could be convinced of that if it was accepted elsewhere and bought us
> something useful.

That part isn't problematic. It's accepted by the C++ standard: the delete operator accepts const pointers. As per the spec, a const object's constness begins after its constructor finishes and ends when its destructor is invoked. (So, for example, a destructor is perfectly free to call non-const members even when it's being invoked for a const object.) Reasoning and history can be found at http://stackoverflow.com/questions/2271046/if-changing-a-const-object-is-undefined-behavior-then-how-do-constructors-and-de?lq=1

But it sounds like you don't see enough win even apart from that. Apparently, botond took a stab at fixing the underlying problem, which is that XPCOM ref counting should already work the way I was making JSPrincipals work here. But that was a rat's nest. If that were done, I think this could be done in an obvious and clean manner.