Bug 190756 - TemplateObject passed to template literal tags are not always identical for the same source location.
Summary: TemplateObject passed to template literal tags are not always identical for t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari 12
Hardware: All Unspecified
: P2 Critical
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
: 193764 (view as bug list)
Depends on: 197671
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-19 11:10 PDT by Justin Fagnani
Modified: 2019-05-07 15:16 PDT (History)
14 users (show)

See Also:


Attachments
Patch (51.13 KB, patch)
2019-05-03 21:06 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (51.13 KB, patch)
2019-05-03 21:08 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (51.48 KB, patch)
2019-05-03 21:29 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (65.28 KB, patch)
2019-05-03 21:50 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (70.04 KB, patch)
2019-05-03 22:10 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (71.20 KB, patch)
2019-05-03 23:16 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.87 MB, application/zip)
2019-05-04 01:43 PDT, Build Bot
no flags Details
Patch (71.22 KB, patch)
2019-05-04 15:53 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.40 MB, application/zip)
2019-05-04 20:12 PDT, Build Bot
no flags Details
Patch (73.66 KB, patch)
2019-05-07 14:06 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fagnani 2018-10-19 11:10:03 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-10-19 12:32:35 PDT
<rdar://problem/45413383>
Comment 2 Ryosuke Niwa 2018-10-19 12:49:56 PDT
Note this is affecting Google's Polymer library.
Comment 3 Yusuke Suzuki 2018-10-19 12:52:57 PDT
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?
Comment 4 Justin Fagnani 2018-10-19 13:02:01 PDT
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?
Comment 5 Yusuke Suzuki 2018-10-19 13:04:22 PDT
(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 :)
Comment 6 Yusuke Suzuki 2018-10-20 10:24:47 PDT
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.
Comment 7 Yusuke Suzuki 2018-10-22 08:28:25 PDT
Working on this. My plan is adding a new side data which holds template object information to ScriptExecutable.
Comment 8 Justin Fagnani 2019-02-04 12:25:47 PST
Yusuke Suzuki, any update here? Thanks!
Comment 9 Ryosuke Niwa 2019-02-04 14:42:35 PST
*** Bug 193764 has been marked as a duplicate of this bug. ***
Comment 10 Sergey Rubanov 2019-02-28 12:33:19 PST
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
Comment 12 Justin Ridgewell 2019-03-21 19:06:49 PDT
This is still broken in TP 78
Comment 13 Yusuke Suzuki 2019-05-03 21:06:12 PDT
Created attachment 369041 [details]
Patch
Comment 14 Yusuke Suzuki 2019-05-03 21:08:02 PDT
Created attachment 369042 [details]
Patch
Comment 15 Yusuke Suzuki 2019-05-03 21:29:53 PDT
Created attachment 369043 [details]
Patch
Comment 16 Yusuke Suzuki 2019-05-03 21:50:07 PDT
Created attachment 369047 [details]
Patch
Comment 17 Yusuke Suzuki 2019-05-03 22:10:26 PDT
Created attachment 369049 [details]
Patch
Comment 18 Yusuke Suzuki 2019-05-03 23:16:37 PDT
Created attachment 369058 [details]
Patch
Comment 19 Build Bot 2019-05-04 01:43:55 PDT
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
Comment 20 Build Bot 2019-05-04 01:43:57 PDT
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
Comment 21 Yusuke Suzuki 2019-05-04 15:53:36 PDT
Created attachment 369074 [details]
Patch
Comment 22 Build Bot 2019-05-04 20:12:13 PDT
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
Comment 23 Build Bot 2019-05-04 20:12:15 PDT
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
Comment 24 Yusuke Suzuki 2019-05-05 01:02:23 PDT
(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 25 Saam Barati 2019-05-06 12:26:37 PDT
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 26 Yusuke Suzuki 2019-05-06 13:35:57 PDT
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.
Comment 27 Yusuke Suzuki 2019-05-06 14:33:40 PDT
Committed r244978: <https://trac.webkit.org/changeset/244978>
Comment 28 Ryan Haddad 2019-05-06 16:06:16 PDT
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
Comment 29 Yusuke Suzuki 2019-05-07 11:31:16 PDT
(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!
Comment 30 Yusuke Suzuki 2019-05-07 12:01:26 PDT
(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.
Comment 31 WebKit Commit Bot 2019-05-07 12:22:37 PDT
Re-opened since this is blocked by bug 197671
Comment 32 Yusuke Suzuki 2019-05-07 14:06:01 PDT
Created attachment 369324 [details]
Patch
Comment 33 Yusuke Suzuki 2019-05-07 15:16:09 PDT
Committed r245040: <https://trac.webkit.org/changeset/245040>