Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Remove JS_{Is,Make}SystemObject API

RESOLVED FIXED in Firefox 16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
2 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)

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.
Blocks: 774633
Created attachment 642943 [details] [diff] [review]
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.
Attachment #642943 - Flags: review?(bzbarsky)
Created attachment 642944 [details] [diff] [review]
Part 2 - Remove JS_{Is,Make}SystemObject API. v1
Attachment #642944 - Flags: review?(luke)

Comment 3

5 years ago
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 4

5 years ago
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+
(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

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

5 years ago
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+
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8e98f6d6566a
Whiteboard: [js:t]
No longer blocks: 774633
Depends on: 774633
Blocks: 771354
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad52e488977
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/134557d57e6e
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
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Pushed to m-a:
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=b8ed43695721
status-firefox17: --- → fixed
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=6fbaf3aed7a0
status-firefox16: --- → fixed

Updated

5 years ago
Duplicate of this bug: 574086
Whiteboard: [js:t] → [js:t][qa-]

Updated

2 years ago
Duplicate of this bug: 574378
You need to log in before you can comment on or make changes to this bug.