1559123 - Template literal in function not cached due to function relazification

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Andrea Giammarchi]

Attachment: the template literal is collected while it shouldn't

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

I have visited this JS Bin:
https://jsbin.com/peniburaho/1/edit?html,console,output

which was used to file a similar bug in WebKit/Safari, where the WeakSet was also involved:
https://bugs.webkit.org/show_bug.cgi?id=190756

Actual results:

Accordingly with Note 2 of ECMAScript Language Specification:
http://www.ecma-international.org/ecma-262/#sec-gettemplateobject

Each TemplateLiteral in the program code of a realm is associated with a unique template object that is used in the evaluation of tagged Templates (12.2.9.6).
The template objects are frozen and the same template object is used each time a specific tagged Template is evaluated.
Whether template objects are created lazily upon first evaluation of the TemplateLiteral or eagerly prior to first evaluation is an implementation choice that is not observable to ECMAScript code.

However, as you can see in the JS Bin page, the template literal might be collected without any reason, since it's still both used, and it should be the exact same each time.

Expected results:

A template literal used as WeakMap key should never be collected if the code relying on such key associated it to some heavy computation/transformation for all static parts of the literal.

Specially with libraries that associate templates to DOM nodes, and perfom heavy scans over template literals, this behavior is disastrous prone, also because the related DOM might leak in a linbo, when the template literal is collected.

Please fix this like WebKit did, and behave like others, otherwise Firefox needs to be penalized in performance, or memory usage, due such issue.

Thank You.

Comment 1

[Andrea Giammarchi]

To provide some extra context: I came from here where I've left my current resolution/workaround to this issue, but this is still a very annoying issue for anything that uses, and trust, template literals uniqueness. https://bugzilla.mozilla.org/show_bug.cgi?id=1514421#c48

Best Regards

Comment 2

[Andrew McCreight [:mccr8]]

You mention weak map keys, but I don't see any call like new WeakMap in there, so I don't quite understand what you are saying.

Comment 3

[Andrea Giammarchi]

(In reply to Andrew McCreight [:mccr8] from comment #2)

You mention weak map keys, but I don't see any call like new WeakMap in there, so I don't quite understand what you are saying.

The original bug was filed to one of my library:
https://github.com/WebReflection/lighterhtml/issues/46

My library uses template literals as WeakMap keys, and it trusts whenever a reference to that key is created once, it can trust every time the same template literal is used to rener its own view (something like this.html<p>template</p>) it populates once its own content.

When the template change, the previous content gets trashed for no reason whatsoever, even if the template literal is used within a method of an object actively live on the dom (Custom Elements, in this case).

Accordingly with specs, not only WeakMap / GC shouldn't be observable, but it is in Firefox, template literals used within non collectable object methods, or closures, should never be collected, otherwise the fallback is to trap these as non Weak Reference, so that all advantages of using a WeakMap are gone.

This is also a very specific issue that doesn't affect any other object, but for some reason template literals are treated more greedy than they should.

Despite all this, the test case should never show the template literal collected, so until GC starts folllowing specs around template literals, it doesn't matter if it's a WeakMap key, or a WeakSet entry, or a reachable callback that supposed to grant its template literal is the same each time it's invoked, by specs.

I hope it's now cleaner what is this bug about: every other browser wouldn't fail neither that linked JS Bin, nor template literals as WeakMap keys.

The example observed in the initial bug they filed me is now using my workaround patch for this issue, so if you want to try it with the old behavior you need to run that codepen with version <= 0.14.2 of that library, and see the button loses its own reference, but it keeps actually living in a limbo.

Anyway, I am sure once that JS Bin test is fixed, all WeakMap related issues will be fixed too, 'cause it means template literals won't ever be collected when surrounding code is reachable and referenced, so that WeakMaps or WeakSets would not ever forget about these.

Comment 4

[Andrew McCreight [:mccr8]]

Ok, thanks for the explanation. I'm going to remove weak map from the bug summary, because it sounds like it is a problem with template literals in general, not just when used as weak map keys. (We've had various problems in the past specifically with things being used as weak map keys, which is what I was initially assuming this was based on the summary.)

Comment 5

[Andrew McCreight [:mccr8]]

There aren't a ton of open bugs related to template literals. I found bug 1403601, which talks about their lifetime, but it is about a spec change that allows reducing the lifetime (though it should still cache it in the case here). Maybe that change already got landed somewhere. I'm not sure who is working on template literals now, if anybody.

Comment 6

[Andrea Giammarchi]

(In reply to Andrew McCreight [:mccr8] from comment #4)

Ok, thanks for the explanation. I'm going to remove weak map from the bug summary, because it sounds like it is a problem with template literals in general, not just when used as weak map keys. (We've had various problems in the past specifically with things being used as weak map keys, which is what I was initially assuming this was based on the summary.)

yes, sorry, I've been through this due my libraries usage of template literals, but the culprit is in how GC handle these in general, not as WeakMap keys (I hope this case is not both issues at once).

Comment 7

[Andrea Giammarchi]

(In reply to Andrew McCreight [:mccr8] from comment #5)

There aren't a ton of open bugs related to template literals. I found bug 1403601, which talks about their lifetime, but it is about a spec change that allows reducing the lifetime (though it should still cache it in the case here). Maybe that change already got landed somewhere. I'm not sure who is working on template literals now, if anybody.

The specs change and its side effects hit me already, and have been described in here:
https://medium.com/@WebReflection/a-tiny-disastrous-ecmascript-change-fadc05c83e69

However, it looks like the current Firefox resolution is greedier than specs themselves suggest, and surely greedier than Chrome, and recently WebKit/Safari, 'cause it's the only one failing with my libraries.

Pleaase also not my libraries are just one example, Polymer team at Google uses tempalte literals too, and so do many others, and having their uniqueness, since making an array of string a unique identifier is neither trivial nor fast to do on the client, is vital for JS / Web itself.

Comment 8

[Andrea Giammarchi]

Pleaase also not => Please also note

Comment 9

[Andrew McCreight [:mccr8]]

FYI, you can actually edit your comments in Bugzilla now, by clicking on the little pencil icon.

Comment 10

[Andrea Giammarchi]

(In reply to Andrew McCreight [:mccr8] from comment #9)

FYI, you can actually edit your comments in Bugzilla now, by clicking on the little pencil icon.

FYI, I don't think so, as there's no such thing on my screen, but I'm sure you can.
https://gist.github.com/WebReflection/a9a9ab943daeaf86eb0e2282d2c68574

Comment 11

[Jason Orendorff [:jorendorff]]

This bug is due to function relazification.

Shell testcase:

const getStrings = (strings, ...values) => strings;
const getRef = () => getStrings`test`;
let c = getRef();
relazifyFunctions(getRef);
assertEq(getRef(), c); // fails

Comment 12

[Andrea Giammarchi]

(In reply to Jason Orendorff [:jorendorff] from comment #11)

This bug is due to function relazification.

awesome finding! do you think that can affect object methods too? thanks a lot!

Comment 13

[Jason Orendorff [:jorendorff]]

Yeah, I think it'll affect all functions.

---

Ashley, I think this predicate needs to be false for functions that contain a tagged template (like getRef in this testcase).

https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/js/src/vm/JSScript.h#2568-2572

Ask jandem for the review, please.

Comment 14

[Ashley Hauck [:khyperia]]

Attachment: Bug 1559123 - Do not relazify scripts with JSOP_CALLSITEOBJ.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e123feb2898
Do not relazify scripts with JSOP_CALLSITEOBJ. r=jandem

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/5e123feb2898