Summary: | TemplateObject passed to template literal tags are not always identical for the same source location. | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Fagnani <justinfagnani> | ||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Critical | CC: | andrea.giammarchi, chi187, commit-queue, ews-watchlist, jridgewell, keith_miller, mark.lam, msaboff, preetshihn, rniwa, saam, webkit-bot-watchers-bugzilla, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | Safari 12 | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 197671 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Justin Fagnani
2018-10-19 11:10:03 PDT
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> |