1803800 - Stop using __proto__ in Firefox code

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gijs (he/him)]

For context, see bug 1770057.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1803800, r?markh!

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1803800, r?mak

Depends on D163931

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1803800, r?mak

Depends on D163932

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1803800 - load browserPlacesViews.js only once and immediately from browser scripts, r?mak!,florian!,Standard8!

Unfortunately lazy script getters load the file once for each accessed symbol, which
redeclares "let" and "const" and "class" variables, which was a problem here. But also,
loading the file multiple times is bad for performance (it is not a module so we
actually reload it) and the "lazy" variables get dereferenced immediately from
markup that is present both in browser.xhtml and the hidden window on macOS, so I
doubt lazy loading was getting us anything performance-wise.

This patch simplifies things by 'just' loading the file from the markup.

Depends on D163933

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1803800, r?pbz!,sgalich!

Depends on D163934

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1803800, r?pbz

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1803800 - followup, actually allow using strictEqual in test_jsctypes.js

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/84d680d1a99e5a28e580b5a4bcfada6322862c0d
https://hg.mozilla.org/mozilla-central/rev/c7e508371e8fcdf397db66ab4f3f018fc6b602f1
https://hg.mozilla.org/mozilla-central/rev/06c21393dcc748b7ed2006c1a550919518630e95
https://hg.mozilla.org/mozilla-central/rev/eabefc1c5d67ae50622e4617f7536f48779a22f0
https://hg.mozilla.org/mozilla-central/rev/b8d121aecea9581c1cc814e54ab1958b5112377f
https://hg.mozilla.org/mozilla-central/rev/e9d0678a18243a625cc255f5ec6f842514aad759
https://hg.mozilla.org/mozilla-central/rev/a7850a6c640bbea189afdf53df02e87f05704548

Comment 9

[Tom Schuster (MoCo)]

I don't understand the motivation for this change. It seems like most of the __proto__ replacements here actually make things worse.

Maybe it isn't really obvious from the other bugs, but __proto__ inside an object literal is not a setter. If we remove the Object.prototype.__proto__ getter/setter in the future those uses will still work. Unless the value on the right can be controlled it should even be preferred over changing the [[Prototype]] later using the Object.setPrototypeOf function. (I think most of the old performance issues with that were fixed though)

Comment 10

[:Gijs (he/him)]

(In reply to Tom Schuster (MoCo) from comment #9)

I don't understand the motivation for this change. It seems like most of the __proto__ replacements here actually make things worse.

That's quite a sweeping claim; it would be useful if you clarified how "most" changes here make things worse.

Maybe it isn't really obvious from the other bugs, but __proto__ inside an object literal is not a setter.

It wasn't obvious, no. Peter did remind me once I had finished writing all the patches you see on this bug and put them up. We had a brief discussion about what to do, in which my impression was that there was no strong reason to prefer __proto__ inside the literal over setPrototypeOf. Given that the patches were written, and did make finding remaining __proto__ cases much easier with simple searchfox queries, and in some cases switched to using class which is probably what we want to do longer term for readability/maintainability/documentation reasons anyway, I chose to continue getting to the point of landing this patchset. I did note this on the bigger review request that was still outstanding at the time (https://phabricator.services.mozilla.com/D163931#5388780) and left the choice to the reviewer.

I would have preferred to just switch everything to class immediately, but I ran into enough issues doing that just with the places code (there are implications around how super works, and how initialization of this works vs. ordering of constructors in the class/prototype chain, as well as plain lexical declaration, that broke a number of existing assumptions in the code) that I ended up postponing that kind of change for the other modules touched. We are likely to still do that kind of refactoring in other, non-security bugs though, e.g. bug 1804264.

If we remove the Object.prototype.__proto__ getter/setter in the future those uses will still work. Unless the value on the right can be controlled it should even be preferred over changing the [[Prototype]] later using the Object.setPrototypeOf function. (I think most of the old performance issues with that were fixed though)

I'm having trouble parsing this - it sounds like you're both saying that there won't be a perf difference anymore and that we should still prefer the "old" way of doing this, despite the fact that I can't even read the documentation for __proto__ on MDN without scrolling, because my entire screen is filled with red warnings saying "do not use this" and explicitly recommending getPrototypeOf and setPrototypeOf instead.

If it's really the case that using __proto__ in an object literal is objectively better, then we can try reverting some of the patches here in a separate bug, but then we should probably also revisit that documentation...

Finally, I'll note that I would say most of the code changed here is not hot/performance-sensitive in any way, perhaps being run once per window (and in fact, the places code is now generally being run fewer times...). The exception is the sync code, I think. I doubt that we have existing infrastructure that measures its performance, but if you think that this might have caused a serious performance regression there, let's file a follow-up bug and investigate that?

Comment 11

[Tom Schuster (MoCo)]

That's quite a sweeping claim; it would be useful if you clarified how "most" changes here make things worse.

Sorry. I only scrolled through https://hg.mozilla.org/mozilla-central/rev/a7850a6c640bbea189afdf53df02e87f05704548 which seems to show a lot of places where we are replacing __proto__ inside an object literal with a call to Object.setPrototyeOf. I see now that some of the later patches improve the current situation by using class .. extends.

It wasn't obvious, no. Peter did remind me once I had finished writing all the patches you see on this bug and put them up. We had a brief discussion about what to do, in which my impression was that there was no strong reason to prefer proto inside the literal over setPrototypeOf. ...

Other people might disagree, but I strongly prefer class > __proto__ inside object literal > Object.setPrototypeOf > proto (don't use this obviously). Using class instead of proto would be very nice, but I also see how updating some of this existing code would be tedious.

I can see how removing all instances of __proto__ would make it a lot easier to to audit everything. I just hope we don't end up with a situation where we remove proto but instead have Object.setPrototypeOf everywhere. Have we considered using a linter instead that allows it inside an object literal?

If it's really the case that using proto in an object literal is objectively better, then we can try reverting some of the patches here in a separate bug, but then we should probably also revisit that documentation...

Yeah I think it's not obvious to most people how proto inside an object literal is fundamentally different from calling the setter/getter. I think warning people away from it and to Object.setPrototypeOf is probably a mistake for that specific case. We should naturally always recommend to use class instead. Maybe other people also just have a different mental model.

Comment 12

[Jan de Mooij [:jandem]]

I don't have strong opinions on this. I agree with Tom that __proto__ in object literals is fine and a bit more concise (this also came up when we discussed this originally after Pwn2Own). Using Object.setPrototypeOf should be fine though, and might be a bit more explicit. Proto mutation no longer has the old performance cliffs fortunately.