Bug 190756 – TemplateObject passed to template literal tags are not always identical for the same source location.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Fagnani]

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.

[Radar WebKit Bug Importer]

<rdar://problem/45413383>

[Ryosuke Niwa]

Note this is affecting Google's Polymer library.

[Yusuke Suzuki]

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?

[Justin Fagnani]

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?

[Yusuke Suzuki]

(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 :)

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Working on this. My plan is adding a new side data which holds template object information to ScriptExecutable.

[Justin Fagnani]

Yusuke Suzuki, any update here? Thanks!

[Ryosuke Niwa]

*** Bug 193764 has been marked as a duplicate of this bug. ***

[Sergey Rubanov]

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

[Sergey Rubanov]

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

[Justin Ridgewell]

This is still broken in TP 78

[Yusuke Suzuki]

Created attachment 369041 [details]
Patch

[Yusuke Suzuki]

Created attachment 369042 [details]
Patch

[Yusuke Suzuki]

Created attachment 369043 [details]
Patch

[Yusuke Suzuki]

Created attachment 369047 [details]
Patch

[Yusuke Suzuki]

Created attachment 369049 [details]
Patch

[Yusuke Suzuki]

Created attachment 369058 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 369074 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

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

[Saam Barati]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r244978: <https://trac.webkit.org/changeset/244978>

[Ryan Haddad]

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

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 197671

[Yusuke Suzuki]

Created attachment 369324 [details]
Patch

[Yusuke Suzuki]

Committed r245040: <https://trac.webkit.org/changeset/245040>