Try to change Intl prototype object to plain objects

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript: Internationalization API
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox53 wontfix, firefox54 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
This is similar to bug 797686. 

From Waldo in bug 1331473, comment #2:
> As to the prototypes being lazy-created, I wonder if they could be made into
> plain objects now.  Probably worth a try fixing that and updating the spec, too.

and bug 1331474, comment #2:
> Why don't we try getting the spec changed to make these plain objects, rather
> than deferring logic and violating the spec?


Chrome/V8 already treats Intl prototypes as plain objects, so changing the Intl spec seems to be possible without breaking web-compat. Let's try this out and if we don't break websites, we can propose the spec change to TC39.

Comment 1

2 years ago
Bug 1328386 and/or other bugs in this area probably stand in the way of doing this, right?  (Or at least they touch code very close to this, ergo conflict with it at the patch level.)  I'd be churning out the patch now if it weren't for that.  So I'm going to assume we'll get those roadblocks out of the way first, and only then will we want to go about fixing this.
(Assignee)

Comment 2

2 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> Bug 1328386 and/or other bugs in this area probably stand in the way of
> doing this, right? 

Yes, exactly. I've already prepared a patch - it's super easy because we just need to remove code. And now we can also remove the undefined checks in the finalizers (bug 949220), because that one was only necessary for Intl prototype objects. My plan for bug 1328386 was to wait until bug 1320408 lands (just happened) because this requires some rebasing, address the remaining review comments and request checkin after the merge on Monday.
(Assignee)

Comment 3

2 years ago
Created attachment 8829516 [details] [diff] [review]
bug1332604-part1.patch
Attachment #8829516 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

2 years ago
Created attachment 8829517 [details] [diff] [review]
bug1332604-part2.patch
Attachment #8829517 - Flags: review?(jwalden+bmo)

Updated

2 years ago
Attachment #8829516 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 5

2 years ago
(Note to self: Don't forget to update the first patch to include the jit-test changes; https://hg.mozilla.org/try/rev/ca9fe463fcb3c09e07a995577e54c4dfe2b5e965
)

Comment 6

2 years ago
Comment on attachment 8829517 [details] [diff] [review]
bug1332604-part2.patch

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

Eeugh.  This is really tricky.  :-\  The checks were needed because the prototypes were allocated as blank prototypes, then they were fallibly setSingleton'd which might fail before the reserved slots could be filled in.  Not so much because of prototypes being allocated, as because prototypes imply singleton funny business.  I kind of want a comment or assertion or something by this, but you can't really have one, can you?  Intl objects can be singletons, and you can make them singletons, you just can't do it before the reserved slots are set.  And the reserved slots *are* set, for every other Intl object creation mechanic.  And that's the normal way object finalizers/allocation works, it's just prototypes with their singleton-ness that are oddball.

So, okay -- just wish I hadn't had to stare at this so long to re-understand/self-justify it.
Attachment #8829517 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 7

2 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> I kind of want a comment or assertion or something by this, but you can't really have one, can you? 

No, I don't think so. We could use setPrivate() instead of slots, because private data is always initialized with nullptr instead of UndefinedValue(), but I'm not sure it's necessary/useful to have this kind of protection...
(Assignee)

Comment 8

2 years ago
Created attachment 8830703 [details] [diff] [review]
bug1332604-part1.patch

Changed part 1 to update two jit-tests which expected Intl prototype objects are initialized Intl objects. Carrying r+.
Attachment #8829516 - Attachment is obsolete: true
Attachment #8830703 - Flags: review+
(Assignee)

Comment 9

2 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83ad8b9e09f3739283a72a801d975fb85a81fd0a

(Intermittent cgc failures in that try push were handled by bug 1328386, patch part 9.)
Keywords: checkin-needed

Comment 10

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a67ac2fe858f
Part 1: Change Intl prototypes to plain objects. r=Waldo
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a67ac2fe858f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1334573
Does this need to be considered for backport?
Flags: needinfo?(andrebargull)
Blocks: 1314354
(Assignee)

Comment 14

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> Does this need to be considered for backport?

No, because the spec change isn't yet approved (https://github.com/tc39/ecma402/issues/122).
Flags: needinfo?(andrebargull)
status-firefox52: --- → wontfix
status-firefox53: affected → wontfix
You need to log in before you can comment on or make changes to this bug.