The first argument to template literal tags should be identical for repeated evaluations of the same template literal: https://www.ecma-international.org/ecma-262/9.0/index.html#sec-gettemplateobject This is usually the case in WebKit, but sometimes a new template object will be passed. I made a reproduction here: https://jsbin.com/peniburaho/1/edit?html,console,output Usually, sometime in the first 20 iterations, the message "New ref 😢" will be written to the console, indicating that the identity of the template object has changed. On some runs I don't see the message. I've tested this on Safari 12 and Safari TP. We have multiple user reports of the issue on iOS too. We have not seen similar reports for other browsers. This is a *critical* problem for libraries that use the identity of the template object as a cache key to keep from doing duplicated work. lit-html for example uses the template object to not only cache expensive template preparation work, but to determine when a template is being re-rendered to a location in DOM vs. when a new template is being rendered to that location. If it incorrectly determines that a new template is being rendered, it destroys and recreates the DOM, causing focus, scroll position and other DOM state to be lost.
<rdar://problem/45413383>
Note this is affecting Google's Polymer library.
Hmm, I can't access to https://jsbin.com/peniburaho/1/edit?html,console,output now (is jsbin.com down?). Could you attach the test to this bug?
Yeah, jsbin looks down. I don't have the tab open anymore, but the code was like this: ``` const getStrings = (strings, ...values) => strings; const getRef = () => getStrings`test`; const ref = getRef(); let wait = 0; const go = () => { const newRef = getRef(); if (ref !== newRef) { console.log('New ref 😢'); } console.log(`wait ${wait}`); wait += 100; setTimeout(go, wait); }; go(); ``` Trying this now in the console just now I haven't seen the bug happen. Maybe the source needs to be in a script tag to trigger it?
(In reply to Justin Fagnani from comment #4) > Yeah, jsbin looks down. I don't have the tab open anymore, but the code was > like this: > > ``` > const getStrings = (strings, ...values) => strings; > const getRef = () => getStrings`test`; > const ref = getRef(); > let wait = 0; > > const go = () => { > const newRef = getRef(); > if (ref !== newRef) { > console.log('New ref 😢'); > } > console.log(`wait ${wait}`); > wait += 100; > setTimeout(go, wait); > }; > go(); > ``` > > Trying this now in the console just now I haven't seen the bug happen. Maybe > the source needs to be in a script tag to trigger it? Super nice! Reproduced. Investigating :)
Found the issue. CodeBlock::finishCreation perform linking. In this phase, we materialize an actual object for tagged template call site from unlinked tagged template descriptor. The problem is that the same JSFunction can discard and recreate CodeBlock multiple times. Everytime we create a new CodeBlock for the same JSFunction, we create a new object, which will break the identity.
Working on this. My plan is adding a new side data which holds template object information to ScriptExecutable.
Yusuke Suzuki, any update here? Thanks!
*** Bug 193764 has been marked as a duplicate of this bug. ***
Was this fixed? Can't reproduce bug on Version 12.1 (14607.1.35.1) and Release 76 (Safari 12.2, WebKit 14608.1.6.2) https://github.com/kangax/compat-table/pull/1427#discussion_r261365767
Actually it seems that it's not fixed https://user-images.githubusercontent.com/1507086/53596846-c6149a80-3bb1-11e9-845a-cc318123753b.png https://user-images.githubusercontent.com/1507086/53597245-be092a80-3bb2-11e9-878b-bc967e76e7aa.png
This is still broken in TP 78
Created attachment 369041 [details] Patch
Created attachment 369042 [details] Patch
Created attachment 369043 [details] Patch
Created attachment 369047 [details] Patch
Created attachment 369049 [details] Patch
Created attachment 369058 [details] Patch
Comment on attachment 369058 [details] Patch Attachment 369058 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12097754 New failing tests: legacy-animation-engine/imported/blink/compositing/layer-creation/incremental-destruction.html svg/repaint/remove-background-property-on-root.html
Created attachment 369066 [details] Archive of layout-test-results from ews211 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 369074 [details] Patch
Comment on attachment 369074 [details] Patch Attachment 369074 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12102284 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
Created attachment 369081 [details] Archive of layout-test-results from ews211 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
(In reply to Build Bot from comment #22) > Comment on attachment 369074 [details] > Patch > > Attachment 369074 [details] did not pass win-ews (win): > Output: https://webkit-queues.webkit.org/results/12102284 > > New failing tests: > security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star. > html This is unrelated.
Comment on attachment 369074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369074&action=review r=me > Source/JavaScriptCore/ChangeLog:16 > + But the top-level one is not created by CodeBlock. This top-level ScriptExecutable is well-aligned to the Script itself. The top-level ScriptExecutable has now HashMap, has now => now has > Source/JavaScriptCore/runtime/FunctionExecutable.h:339 > + // FIXME: We can merge rareData pointer and top-level executable pointer. First time, setting parent. If RareData is required, materialize RareData, swap it, and store > + // top-level executable pointer inside RareData. If you think we should do this, please file a bug and link it here. Otherwise, if you don't think it's important, let's remove this FIXME > Source/JavaScriptCore/runtime/ModuleProgramExecutable.cpp:96 > +auto ModuleProgramExecutable::ensureTemplateObjectMap(VM&) -> TemplateObjectMap& > +{ > + if (m_templateObjectMap) > + return *m_templateObjectMap; > + auto result = std::make_unique<TemplateObjectMap>(); > + WTF::storeStoreFence(); > + m_templateObjectMap = WTFMove(result); > + return *m_templateObjectMap; > +} You have this more or less implemented four times. Maybe you can do: static TemplateObjectMap& ensureTemplateObjectMapImpl(std::unique_ptr<TemplateObjectMap>& dst) { if (dst) return *dst; auto result = std::make_unique<TemplateObjectMap>(); WTF::storeStoreFence(); dst = WTFMove(result); return *dst; } And then call this as appropriate from four places. > Source/JavaScriptCore/runtime/ScriptExecutable.cpp:463 > + RETURN_IF_EXCEPTION(scope, nullptr); I think you have a bug if this exception is thrown. Because the next time this is called, you'll end up returning a null array. I think you might want the above "!result.isNewEntry" to also check the value in the hashmap. Would be good to add a test if it's possible. > Source/JavaScriptCore/runtime/ScriptExecutable.cpp:480 > + ASSERT(type() == ModuleProgramExecutableType); > + return static_cast<ModuleProgramExecutable*>(this)->ensureTemplateObjectMap(vm); nit: this styling feels weird. Why not just ASSERT_NOT_REACHED() in "default" case and move this logic up to ModuleProgramExecutableType?
Comment on attachment 369074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369074&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:16 >> + But the top-level one is not created by CodeBlock. This top-level ScriptExecutable is well-aligned to the Script itself. The top-level ScriptExecutable has now HashMap, > > has now => now has Fixed. >> Source/JavaScriptCore/runtime/FunctionExecutable.h:339 >> + // top-level executable pointer inside RareData. > > If you think we should do this, please file a bug and link it here. Otherwise, if you don't think it's important, let's remove this FIXME Added URL. >> Source/JavaScriptCore/runtime/ModuleProgramExecutable.cpp:96 >> +} > > You have this more or less implemented four times. Maybe you can do: > > static TemplateObjectMap& ensureTemplateObjectMapImpl(std::unique_ptr<TemplateObjectMap>& dst) > { > if (dst) > return *dst; > auto result = std::make_unique<TemplateObjectMap>(); > WTF::storeStoreFence(); > dst = WTFMove(result); > return *dst; > } > > And then call this as appropriate from four places. Fixed. >> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:463 >> + RETURN_IF_EXCEPTION(scope, nullptr); > > I think you have a bug if this exception is thrown. Because the next time this is called, you'll end up returning a null array. I think you might want the above "!result.isNewEntry" to also check the value in the hashmap. Would be good to add a test if it's possible. Nice catch! Yeah, correct. We can do, if (JSArray* array = result.iterator->value.get()) return array; instead. Fixed. >> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:480 >> + return static_cast<ModuleProgramExecutable*>(this)->ensureTemplateObjectMap(vm); > > nit: this styling feels weird. Why not just ASSERT_NOT_REACHED() in "default" case and move this logic up to ModuleProgramExecutableType? This is because we need to return some `TemplateObjectMap&` even in ASSERT_NOT_REACHED() case to make the compiler happy. But we do not have a good thing to be returned at `default:` case. That's why I merge default and the last case, and putting ASSERT inside it. If we change TemplateObjectMap& to TemplateObjectMap*, we can return `nullptr` from the default: clause, but I prefer using TemplateObjectMap& than TemplateObjectMap* here since ensureXXX should return non-null thing.
Committed r244978: <https://trac.webkit.org/changeset/244978>
This change caused two test262 failures: ---------------NEW FAILING TESTS SUMMARY--------------- FAIL test/language/expressions/tagged-template/chained-application.js (default) Full Output: Exception: Test262Error: Expected SameValue(«x», «y») to be true FAIL test/language/expressions/tagged-template/chained-application.js (strict mode) Full Output: Exception: Test262Error: Expected SameValue(«x», «y») to be true https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20Test262%20%28Tests%29/builds/2203
(In reply to Ryan Haddad from comment #28) > This change caused two test262 failures: > > ---------------NEW FAILING TESTS SUMMARY--------------- > > FAIL test/language/expressions/tagged-template/chained-application.js > (default) > Full Output: > Exception: Test262Error: Expected SameValue(«x», «y») to be true > > FAIL test/language/expressions/tagged-template/chained-application.js > (strict mode) > Full Output: > Exception: Test262Error: Expected SameValue(«x», «y») to be true > > > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Debug%20Test262%20%28Tests%29/builds/2203 Look into it, thanks!
(In reply to Yusuke Suzuki from comment #29) > (In reply to Ryan Haddad from comment #28) > > This change caused two test262 failures: > > > > ---------------NEW FAILING TESTS SUMMARY--------------- > > > > FAIL test/language/expressions/tagged-template/chained-application.js > > (default) > > Full Output: > > Exception: Test262Error: Expected SameValue(«x», «y») to be true > > > > FAIL test/language/expressions/tagged-template/chained-application.js > > (strict mode) > > Full Output: > > Exception: Test262Error: Expected SameValue(«x», «y») to be true > > > > > > https://build.webkit.org/builders/ > > Apple%20High%20Sierra%20Debug%20Test262%20%28Tests%29/builds/2203 > > Look into it, thanks! OK, bug is found, I'll quickly upload the follow-up fix.
Re-opened since this is blocked by bug 197671
Created attachment 369324 [details] Patch
Committed r245040: <https://trac.webkit.org/changeset/245040>