Implement "global" proposal

ASSIGNED
Assigned to

Status

()

P5
normal
ASSIGNED
2 years ago
4 months ago

People

(Reporter: anba, Assigned: Waldo)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-needed})

Trunk
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
"global" [1] is currently a stage 3 proposal [2, 3].

test262 already tests for the new "global" property [4] and we're currently failing some tests because of this.


[1] https://github.com/tc39/proposal-global
[2] https://github.com/tc39/proposals
[3] https://github.com/tc39/ecma262/pull/702
[4] https://github.com/tc39/test262/pull/765
(Assignee)

Comment 1

2 years ago
Created attachment 8815143 [details] [diff] [review]
Patch

#jslang suckered me into it.
Attachment #8815143 - Flags: review?(andrebargull)
(Assignee)

Updated

2 years ago
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED

Comment 2

2 years ago
<3 Thanks!
(Reporter)

Comment 3

2 years ago
Comment on attachment 8815143 [details] [diff] [review]
Patch

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

Let's find out if this breaks any websites! :-D

::: js/src/tests/ecma_2017/Global/global-property-enumeration.js
@@ +28,5 @@
> +assertEq(desc.configurable, true);
> +assertEq(desc.writable, true);
> +assertEq(desc.value, this);
> +
> +global = 42;

The other test case already tests that `global = 42` works, should this one instead test with `this.global = 42`?
Attachment #8815143 - Flags: review?(andrebargull) → review+
(Assignee)

Comment 4

2 years ago
Patch works fine for shell, doesn't work for browser.  Turns out I need a ToWindowProxyIfWindow in there.  But even that's not enough, because in the browser embedding, nsGlobalWindow::SetNewDocument invokes Object class init (in the process of setting up the window's prototype chain) before it creates the WindowProxy wrapper and calls js::SetWindowProxy to store the wrapper in the WINDOW_PROXY slot in the Window.

The right fix to me seems to be to refactor things so the slot's filled early enough that Object class init doesn't have these oddities to deal with.  But SetNewDocument looks an awful lot like Mos Eisley -- a wretched hive of scum and villainy -- and doesn't look particularly refactorable, nor does it seem like a good use of time to do so.  (I want to refactor global object creation at *some* point, so that embeddings can basically declaratively specify what they want their global object to look like, including its prototype chain.  So I'll probably get back to this eventually.  Just, not now.)

With the Object init approach out, I stole the approach evilpie mentioned on IRC (aside: it's truly awful that there are multiple ways to define a new one-off global property), fixed the one moderately obvious bug in it, and am tryservering it now.  Will post for a fresh review if it comes back okay.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7cf02615566f272bd0ad7a89c865262a2a2ce62
(Assignee)

Comment 5

2 years ago
Created attachment 8816662 [details] [diff] [review]
Patch, v2

Pretty sure jandem dealt with the ToWindow* gunk at one point, so let's throw this at him this time around.
Attachment #8816662 - Flags: review?(jdemooij)
(Assignee)

Updated

2 years ago
Attachment #8815143 - Attachment is obsolete: true
Comment on attachment 8816662 [details] [diff] [review]
Patch, v2

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

LGTM.
Attachment #8816662 - Flags: review?(jdemooij) → review+
Reminder to land this.
Keywords: dev-doc-needed

Comment 8

2 years ago
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d37fc752b0af
Implement a global 'global' property whose value is the global object.  r=jandem

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d37fc752b0af
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 10

2 years ago
Yay! When will this land in nightly and stable?
(In reply to ljharb from comment #10)
> Yay! When will this land in nightly and stable?

It will be in today's Nightly. Firefox 53 will be released April 18th, according to https://wiki.mozilla.org/RapidRelease/Calendar
Depends on: 1325907

Updated

2 years ago
Depends on: 1326032
(Assignee)

Updated

2 years ago
Depends on: 1328218
Let's back this out. This is not web-compatible.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8823427 [details] [diff] [review]
Backout
Attachment #8823427 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 14

2 years ago
Comment on attachment 8823427 [details] [diff] [review]
Backout

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

::: js/src/jsapi.cpp
@@ +1076,5 @@
>      // Object.prototype yet. Force it now.
> +    if (!global->getOrCreateObjectPrototype(cx))
> +        return false;
> +
> +    return true;

Mind also landing a separate patch, rs=me, that undoes this undoing, and keeps |global| as a Handle?
Attachment #8823427 - Flags: review?(jwalden+bmo) → review+

Comment 15

2 years ago
Is there a way we could still include `global` behind a flag in nightly, so that continued testing can be done as sites fix their issues?
(Assignee)

Comment 16

2 years ago
Sure, we could add something to CompartmentOptions to frob it on, easily enough.  Invoking the frob other than as an all-or-nothing approach, tho, seems difficult.  I'm not sure exactly how useful such a thing is.  We've used such frobs to feature-guard stuff in progress, like some Intl functionality, but that was only with the aim of having an opt-in in shell tests to enable it.  And obviously, we can't expose such a thing to web content.

Comment 17

2 years ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa5f92e32f1d
Backout the global property propsal. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/89388a238c25
Keep nicer code in JS_ResolveStandardClass. rs=Waldo
How about enabling this just for 'strict' contexts, wouldn't it be more compatible ir current web content?

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aa5f92e32f1d
https://hg.mozilla.org/mozilla-central/rev/89388a238c25
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
status-firefox53: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla53 → ---
Status: REOPENED → ASSIGNED
After implementing it in Safari Tech Preview 21
> Added support for the global property on the global object (r210052)
https://webkit.org/blog/7265/release-notes-for-safari-technology-preview-21/

It has been reverted for Web Compatibility reasons
See https://bugs.webkit.org/show_bug.cgi?id=166915

Ryosuke Niwa said
> We had to rollout this feature it broke Polymer tests.
https://bugs.webkit.org/show_bug.cgi?id=165171#c22

Updated

9 months ago
Depends on: 1422557
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.