774607 - Remove JS_{Is,Make}SystemObject API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

We don't need it in gecko anymore. It's public API though, so maybe other people are using it?

I'm going to post two patches, one of which removes usage from gecko and the other of which removes it from JSAPI. I'll let the JS folks decide whether to take the second.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove use of JS_{Is,Make}SystemObject from Gecko. v1

We currently set this for system globals and anything whose parent
chain leads to a system global. Maybe this was relevant before, but
with CPG this is just equivalent to asking whether the object is in
a system compartment. And the only place where we _check_ this bit
is immediately after checking for a system compartment, in
WrapperFactory. So AFAICT this can go away entirely.

Flagging bz for review, since I've pretty much maxed out mrbkap's review queue. I think this code is verifiably correct by inspection.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove JS_{Is,Make}SystemObject API. v1

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642943 [details] [diff] [review]
Part 1 - Remove use of JS_{Is,Make}SystemObject from Gecko. v1

I think this really needs to be on Blake's plate.  I have too many pending reviews to spend the time sorting through this code that I'd need to review this on a timely basis.

That said, in the old setup you could have non-system objects in a system-principal compartment (e.g. every single piece of in-tab system-principal UI we have).  Is that still the case after this patch?  Was that a feature or a bug?

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 642944 [details] [diff] [review]
Part 2 - Remove JS_{Is,Make}SystemObject API. v1

Sweet!  The magical mysterious system bit has long annoyed us.

Instead of the "// Free bit here!" comment, could you just compact the values?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 642943 [details] [diff] [review]
> Part 1 - Remove use of JS_{Is,Make}SystemObject from Gecko. v1
> 
> I think this really needs to be on Blake's plate.  I have too many pending
> reviews to spend the time sorting through this code that I'd need to review
> this on a timely basis.

Ok, sure. Though I maintain that this patch is reviewable by inspection.

> That said, in the old setup you could have non-system objects in a
> system-principal compartment (e.g. every single piece of in-tab
> system-principal UI we have).  Is that still the case after this patch?  Was
> that a feature or a bug?

I'm not sure what this would look like. In the current world, we call JS_MakeSystemObject in two scenarios: (a) when creating a global with FLAG_SYSTEM_GLOBAL_OBJECT passed by the caller, and (b) when creating an object parented to a system object. From that, we can deduce that an object is a system object i.f.f. it lives in the scope of a FLAG_SYSTEM_GLOBAL_OBJECT global, which lives in a SystemPrincipal compartment.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I can't review the patch to WrapperFactory.cpp by inspection: you're removing code and not replacing it with an obvious equivalent.

> From that, we can deduce that an object is a system object i.f.f. it lives in the scope
> of a FLAG_SYSTEM_GLOBAL_OBJECT global

Yes.

> which lives in a SystemPrincipal compartment.

Yes.  So before this patch, "system object" implies "system principal compartment".  But the converse is not true: "system principal compartment" does NOT imply "system object".

I have no idea whether that's a problem for this patch or not, because I don't understand the WrapperFactory change.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> But the converse is not true: "system principal compartment"
> does NOT imply "system object".
> 
> I have no idea whether that's a problem for this patch or not, because I
> don't understand the WrapperFactory change.

Right above the else block, the function selects &CrossCompartmentWrapper::singleton if the object lives in a system principal compartment (AccessCheck::isChrome(origin) - I admit here that the context of exactly what this means is just out of the scope of the diff). Then, it does a separate check whereby it examines whether the object's global "IsSystem", and similarly selects CrossCompartmentWrapper::singleton in that case.

> So before this patch, "system object" implies "system principal compartment". 

Given the above, we can conclude that the second block is never reached.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642943 [details] [diff] [review]
Part 1 - Remove use of JS_{Is,Make}SystemObject from Gecko. v1

Ah, much better.  r=me

Comment 9

[Bobby Holley (:bholley)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8e98f6d6566a

Comment 12

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad52e488977
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/134557d57e6e

Comment 13

[Matt Brubeck (:mbrubeck)]

Backed out along with other patches from the same push, because of possibly causing timeouts in 'make check' on Windows:
https://hg.mozilla.org/integration/mozilla-inbound/rev/deeadcce3f64
https://tbpl.mozilla.org/php/getParsedLog.php?id=14652164&tree=Mozilla-Inbound

Comment 14

[Bobby Holley (:bholley)]

Relanded to m-i with the fix for the hanging windows make check:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cfc884e6e414

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/569dc0ef24b2
https://hg.mozilla.org/mozilla-central/rev/6427078e02fe

Comment 16

[Bobby Holley (:bholley)]

Pushed to m-a:
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=b8ed43695721

Comment 17

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=6fbaf3aed7a0