Implement "globalThis" proposal

ASSIGNED
Assigned to

Status

()

P5
normal
ASSIGNED
2 years ago
4 hours ago

People

(Reporter: anba, Assigned: evilpie)

Tracking

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

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 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

Comment 1

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

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

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+

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

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)

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+
(Assignee)

Comment 7

2 years ago
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

Updated

2 years ago
Depends on: 1328218
(Assignee)

Comment 12

2 years ago
Let's back this out. This is not web-compatible.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

2 years ago
Created attachment 8823427 [details] [diff] [review]
Backout
Attachment #8823427 - Flags: review?(jwalden+bmo)
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?
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
Depends on: 1422557
Priority: -- → P5
(Assignee)

Updated

a month ago
Blocks: 1435811
QA Contact: jorendorff
(Assignee)

Comment 21

a month ago
From Alexandre Folle de Menezes in bug 1496748
> The 'global' proposal was renamed to 'globalThis'.  I believe this was
> already implemented with the old name, but disabled due to compatibility
> issues.
> 
> Chrome 70 already has the renamed implementation.
(Assignee)

Updated

a month ago
Duplicate of this bug: 1496748
(Assignee)

Comment 23

9 days ago
Should be easy to implement this. I would volunteer to do it.
(Assignee)

Updated

8 days ago
Assignee: jwalden+bmo → evilpies
(Assignee)

Comment 24

8 days ago
Oh the original patch was actually not quite correct: We would resolve "global" again after it was deleted. Thanks test262 for catching this!
(Assignee)

Comment 25

8 days ago
Created attachment 9023621 [details]
Bug 1317422 - Implement JavaScript globalThis proposal. r?jandem
(Assignee)

Updated

8 days ago
Summary: Implement "global" proposal → Implement "globalThis" proposal

Comment 26

4 hours ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1e6a2dbfa104
Implement JavaScript globalThis proposal. r=jandem
You need to log in before you can comment on or make changes to this bug.