Remove JS_{Is,Make}SystemObject API

RESOLVED FIXED in Firefox 16

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 fixed, firefox17 fixed)

Details

(Whiteboard: [js:t][qa-])

Attachments

(2 attachments)

Assignee

Description

7 years ago
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.
Assignee

Updated

7 years ago
Blocks: 774633
Assignee

Comment 1

7 years ago
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.
Attachment #642943 - Flags: review?(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?
Attachment #642943 - Flags: review?(bzbarsky) → review?(mrbkap)
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?
Attachment #642944 - Flags: review?(luke) → review+
Assignee

Comment 5

7 years ago
(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.
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.
Assignee

Comment 7

7 years ago
(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 on attachment 642943 [details] [diff] [review]
Part 1 - Remove use of JS_{Is,Make}SystemObject from Gecko. v1

Ah, much better.  r=me
Attachment #642943 - Flags: review?(mrbkap) → review+
Whiteboard: [js:t]
Assignee

Updated

7 years ago
No longer blocks: 774633
Depends on: 774633
Assignee

Updated

7 years ago
Blocks: 771354
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
Assignee

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cfc884e6e414
https://hg.mozilla.org/mozilla-central/rev/569dc0ef24b2
https://hg.mozilla.org/mozilla-central/rev/6427078e02fe
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Assignee

Updated

7 years ago
Duplicate of this bug: 574086
Whiteboard: [js:t] → [js:t][qa-]
Duplicate of this bug: 574378
You need to log in before you can comment on or make changes to this bug.