215293 2020-08-07 14:48:39 -0700 REGRESSION(r261159) PokerBros only shows black screen 2020-08-10 11:44:31 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 215316 1 achristensen achristensen commit-queue darin ews-watchlist keith_miller mark.lam msaboff saam simon.fraser tzagallo webkit-bug-importer oldest_to_newest 1678745 0 achristensen 2020-08-07 14:48:39 -0700 REGRESSION(r261159) PokerBros only shows black screen 1678747 1 406216 achristensen 2020-08-07 14:51:09 -0700 Created attachment 406216 Patch 1678753 2 406216 keith_miller 2020-08-07 14:56:17 -0700 Comment on attachment 406216 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406216&action=review r=me with comment. > Source/JavaScriptCore/runtime/JSObject.cpp:519 > + static bool isPokerBros = CFEqual(CFBundleGetIdentifier(CFBundleGetMainBundle()), CFSTR("com.kpgame.PokerBros")) I forget the semantics of this? Is this only run once? I assume so but if not this is probably going to be a perf regression. 1678755 3 achristensen 2020-08-07 14:58:46 -0700 Yes, function scoped static variables are initialized the first time the function is run. After that there is a read and a branch to check that it is initialized, then the stored value is returned. There are no atomic operations here, so this should be as little of a perf regression as possible. 1678759 4 achristensen 2020-08-07 15:09:01 -0700 * because of the compile flags we use in WebKit, there are no atomic operations in this check. Normal C++ does have an atomic check for function scoped static variable initialization. http://trac.webkit.org/r265392 1678760 5 webkit-bug-importer 2020-08-07 15:09:32 -0700 <rdar://problem/66702248> 1678808 6 406216 darin 2020-08-07 16:23:23 -0700 Comment on attachment 406216 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406216&action=review >> Source/JavaScriptCore/runtime/JSObject.cpp:519 >> + static bool isPokerBros = CFEqual(CFBundleGetIdentifier(CFBundleGetMainBundle()), CFSTR("com.kpgame.PokerBros")) > > I forget the semantics of this? Is this only run once? I assume so but if not this is probably going to be a perf regression. Yes, static makes it only get run once. 1679004 7 commit-queue 2020-08-09 21:47:44 -0700 Re-opened since this is blocked by bug 215316 1679061 8 achristensen 2020-08-10 09:03:19 -0700 CFBundleGetIdentifier returns null in jsc, and probably some other non-bundle tools. I need to null check its return value, because passing null to CFEqual causes a crash. Will fix. 1679068 9 406300 achristensen 2020-08-10 09:29:00 -0700 Created attachment 406300 Patch 1679072 10 achristensen 2020-08-10 09:32:00 -0700 http://trac.webkit.org/r265431 1679092 11 406300 simon.fraser 2020-08-10 10:01:56 -0700 Comment on attachment 406300 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406300&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:517 > +inline static bool isPokerBros() This name is a lie, because the function also checks the linked-on-or-after version. A better function name would be needsPokerBrosQuirk(). 1679115 12 406300 darin 2020-08-10 10:49:12 -0700 Comment on attachment 406300 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406300&action=review >> Source/JavaScriptCore/runtime/JSObject.cpp:517 >> +inline static bool isPokerBros() > > This name is a lie, because the function also checks the linked-on-or-after version. A better function name would be needsPokerBrosQuirk(). We should not write "inline" here. It’s not valuable to inline the one-time code into JSObject::toStringName, and in fact might be counterproductive. 1679116 13 406300 darin 2020-08-10 10:51:05 -0700 Comment on attachment 406300 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406300&action=review > Source/JavaScriptCore/ChangeLog:10 > + The PokerBros app has some logic that was broken by the change in behavior of r261159. > + It caused the app do do nothing except show a black screen upon opening. > + Revert to the old behavior for this app until they update to iOS14. Normally we keep the quirks all in one place to help us remember to remove them over time. I worry that this one won’t be remembered. 1679127 14 achristensen 2020-08-10 11:38:30 -0700 "inline" is just a suggestion, and might not even change the output of the compiler. __attribute__((always_inline)) forces the compiler to inline. I will remove the "inline" keyword, but I don't think that is necessary before this is copied to the branch. I'll also rename the function to needsPokerBrosQuirk later. We keep the quirks in the same place in WebCore, but this quirk is needed in JSC because it is in use of the JSC API in the app, not in web content. The JSC team keeps track of these quirks and removes them when they can. 1679129 15 darin 2020-08-10 11:41:13 -0700 (In reply to Alex Christensen from comment #14) > "inline" is just a suggestion Yes, fully aware of that. Just not necessarily a good suggestion in this case. > I don't think that is necessary before > this is copied to the branch. I agree. > We keep the quirks in the same place in WebCore, but this quirk is needed in > JSC because it is in use of the JSC API in the app, not in web content. The > JSC team keeps track of these quirks and removes them when they can. How do we keep track of them? Where are the others? I could imagine putting them all in one source file to keep track of them. 1679130 16 achristensen 2020-08-10 11:44:31 -0700 (In reply to Darin Adler from comment #15) > How do we keep track of them? Where are the others? > > I could imagine putting them all in one source file to keep track of them. They are all in one source file because there is only one right now. Another one was removed in r234894 406216 2020-08-07 14:51:09 -0700 2020-08-10 09:28:59 -0700 Patch bug-215293-20200807145109.patch text/plain 2277 achristensen SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjY1MzkxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDIwLTA4LTA3ICBBbGV4IENocmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+ CisKKyAgICAgICAgUkVHUkVTU0lPTihyMjYxMTU5KSBQb2tlckJyb3Mgb25seSBzaG93cyBibGFj ayBzY3JlZW4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTIxNTI5MworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IFRoZSBQb2tlckJyb3MgYXBwIGhhcyBzb21lIGxvZ2ljIHRoYXQgd2FzIGJyb2tlbiBieSB0aGUg Y2hhbmdlIGluIGJlaGF2aW9yIG9mIHIyNjExNTkuCisgICAgICAgIEl0IGNhdXNlZCB0aGUgYXBw IGRvIGRvIG5vdGhpbmcgZXhjZXB0IHNob3cgYSBibGFjayBzY3JlZW4gdXBvbiBvcGVuaW5nLgor ICAgICAgICBSZXZlcnQgdG8gdGhlIG9sZCBiZWhhdmlvciBmb3IgdGhpcyBhcHAgdW50aWwgdGhl eSB1cGRhdGUgdG8gaU9TMTQuCisKKyAgICAgICAgKiBydW50aW1lL0pTT2JqZWN0LmNwcDoKKyAg ICAgICAgKEpTQzo6bmVlZHNPbGRTdHJpbmdOYW1lKToKKyAgICAgICAgKEpTQzo6SlNPYmplY3Q6 OnRvU3RyaW5nTmFtZSk6CisKIDIwMjAtMDgtMDcgIE1pY2hhZWwgU2Fib2ZmICA8bXNhYm9mZkBh cHBsZS5jb20+CiAKICAgICAgICAgUmVnRXhwIHN0aWNreSBub3QgbWF0Y2hpbmcgYWx0ZXJuYXRl cyBjb3JyZWN0bHksIGlnbm9yaW5nIGxhc3RJbmRleCByZXF1aXJlbWVudApJbmRleDogU291cmNl L0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNPYmplY3QuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTT2JqZWN0LmNwcAkocmV2aXNpb24gMjY1Mzg3KQor KysgU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNPYmplY3QuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC00NCw2ICs0NCwxMCBAQAogI2luY2x1ZGUgIlZNSW5saW5lcy5oIgogI2luY2x1ZGUg PHd0Zi9Bc3NlcnRpb25zLmg+CiAKKyNpZiBQTEFURk9STShJT1MpCisjaW5jbHVkZSA8d3RmL3Nw aS9kYXJ3aW4vZHlsZFNQSS5oPgorI2VuZGlmCisKIG5hbWVzcGFjZSBKU0MgewogCiAvLyBXZSBr ZWVwIHRyYWNrIG9mIHRoZSBzaXplIG9mIHRoZSBsYXN0IGFycmF5IGFmdGVyIGl0IHdhcyBncm93 bi4gV2UgdXNlIHRoaXMKQEAgLTUwOSw5ICs1MTMsMjUgQEAgU3RyaW5nIEpTT2JqZWN0OjpjbGFz c05hbWUoY29uc3QgSlNPYmplYwogICAgIHJldHVybiBpbmZvLT5jbGFzc05hbWU7CiB9CiAKKyNp ZiBQTEFURk9STShJT1MpCitpbmxpbmUgc3RhdGljIGJvb2wgbmVlZHNPbGRTdHJpbmdOYW1lKCkK K3sKKyAgICBzdGF0aWMgYm9vbCBpc1Bva2VyQnJvcyA9IENGRXF1YWwoQ0ZCdW5kbGVHZXRJZGVu dGlmaWVyKENGQnVuZGxlR2V0TWFpbkJ1bmRsZSgpKSwgQ0ZTVFIoImNvbS5rcGdhbWUuUG9rZXJC cm9zIikpCisgICAgICAgICYmIGR5bGRfZ2V0X3Byb2dyYW1fc2RrX3ZlcnNpb24oKSA8IERZTERf SU9TX1ZFUlNJT05fMTRfMDsKKyAgICByZXR1cm4gaXNQb2tlckJyb3M7Cit9CisjZW5kaWYKKwog U3RyaW5nIEpTT2JqZWN0Ojp0b1N0cmluZ05hbWUoY29uc3QgSlNPYmplY3QqIG9iamVjdCwgSlNH bG9iYWxPYmplY3QqIGdsb2JhbE9iamVjdCkKIHsKICAgICBWTSYgdm0gPSBnbG9iYWxPYmplY3Qt PnZtKCk7CisjaWYgUExBVEZPUk0oSU9TKQorICAgIGlmIChVTkxJS0VMWShuZWVkc09sZFN0cmlu Z05hbWUoKSkpIHsKKyAgICAgICAgY29uc3QgQ2xhc3NJbmZvKiBpbmZvID0gb2JqZWN0LT5jbGFz c0luZm8odm0pOworICAgICAgICBBU1NFUlQoaW5mbyk7CisgICAgICAgIHJldHVybiBpbmZvLT5j bGFzc05hbWU7CisgICAgfQorI2VuZGlmCiAgICAgYXV0byBzY29wZSA9IERFQ0xBUkVfVEhST1df U0NPUEUodm0pOwogICAgIGJvb2wgb2JqZWN0SXNBcnJheSA9IGlzQXJyYXkoZ2xvYmFsT2JqZWN0 LCBvYmplY3QpOwogICAgIFJFVFVSTl9JRl9FWENFUFRJT04oc2NvcGUsIFN0cmluZygpKTsK 406300 2020-08-10 09:29:00 -0700 2020-08-10 09:31:53 -0700 Patch bug-215293-20200810092859.patch text/plain 2311 achristensen SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjY1NDI5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDIwLTA4LTEwICBBbGV4IENocmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+ CisKKyAgICAgICAgUkVHUkVTU0lPTihyMjYxMTU5KSBQb2tlckJyb3Mgb25seSBzaG93cyBibGFj ayBzY3JlZW4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTIxNTI5MworCisgICAgICAgIFJldmlld2VkIGJ5IEtlaXRoIE1pbGxlci4KKworICAgICAgICBU aGUgUG9rZXJCcm9zIGFwcCBoYXMgc29tZSBsb2dpYyB0aGF0IHdhcyBicm9rZW4gYnkgdGhlIGNo YW5nZSBpbiBiZWhhdmlvciBvZiByMjYxMTU5LgorICAgICAgICBJdCBjYXVzZWQgdGhlIGFwcCBk byBkbyBub3RoaW5nIGV4Y2VwdCBzaG93IGEgYmxhY2sgc2NyZWVuIHVwb24gb3BlbmluZy4KKyAg ICAgICAgUmV2ZXJ0IHRvIHRoZSBvbGQgYmVoYXZpb3IgZm9yIHRoaXMgYXBwIHVudGlsIHRoZXkg dXBkYXRlIHRvIGlPUzE0LgorCisgICAgICAgICogcnVudGltZS9KU09iamVjdC5jcHA6CisgICAg ICAgIChKU0M6Om5lZWRzT2xkU3RyaW5nTmFtZSk6CisgICAgICAgIChKU0M6OkpTT2JqZWN0Ojp0 b1N0cmluZ05hbWUpOgorCiAyMDIwLTA4LTEwICBZdXN1a2UgU3V6dWtpICA8eXN1enVraUBhcHBs ZS5jb20+CiAKICAgICAgICAgW0pTQ10gSlNGaW5hbE9iamVjdDo6ZmluaXNoQ3JlYXRpb24ncyBB U1NFUlQgaGFzIHN0YWxlIGNvbmRpdGlvbgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvSlNPYmplY3QuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9y dW50aW1lL0pTT2JqZWN0LmNwcAkocmV2aXNpb24gMjY1NDI5KQorKysgU291cmNlL0phdmFTY3Jp cHRDb3JlL3J1bnRpbWUvSlNPYmplY3QuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00NCw2ICs0NCwx MCBAQAogI2luY2x1ZGUgIlZNSW5saW5lcy5oIgogI2luY2x1ZGUgPHd0Zi9Bc3NlcnRpb25zLmg+ CiAKKyNpZiBQTEFURk9STShJT1MpCisjaW5jbHVkZSA8d3RmL3NwaS9kYXJ3aW4vZHlsZFNQSS5o PgorI2VuZGlmCisKIG5hbWVzcGFjZSBKU0MgewogCiAvLyBXZSBrZWVwIHRyYWNrIG9mIHRoZSBz aXplIG9mIHRoZSBsYXN0IGFycmF5IGFmdGVyIGl0IHdhcyBncm93bi4gV2UgdXNlIHRoaXMKQEAg LTUwOSw5ICs1MTMsMjcgQEAgU3RyaW5nIEpTT2JqZWN0OjpjbGFzc05hbWUoY29uc3QgSlNPYmpl YwogICAgIHJldHVybiBpbmZvLT5jbGFzc05hbWU7CiB9CiAKKyNpZiBQTEFURk9STShJT1MpCitp bmxpbmUgc3RhdGljIGJvb2wgaXNQb2tlckJyb3MoKQoreworICAgIGF1dG8gYnVuZGxlSUQgPSBD RkJ1bmRsZUdldElkZW50aWZpZXIoQ0ZCdW5kbGVHZXRNYWluQnVuZGxlKCkpOworICAgIHJldHVy biBidW5kbGVJRAorICAgICAgICAmJiBDRkVxdWFsKGJ1bmRsZUlELCBDRlNUUigiY29tLmtwZ2Ft ZS5Qb2tlckJyb3MiKSkKKyAgICAgICAgJiYgZHlsZF9nZXRfcHJvZ3JhbV9zZGtfdmVyc2lvbigp IDwgRFlMRF9JT1NfVkVSU0lPTl8xNF8wOworfQorI2VuZGlmCisKIFN0cmluZyBKU09iamVjdDo6 dG9TdHJpbmdOYW1lKGNvbnN0IEpTT2JqZWN0KiBvYmplY3QsIEpTR2xvYmFsT2JqZWN0KiBnbG9i YWxPYmplY3QpCiB7CiAgICAgVk0mIHZtID0gZ2xvYmFsT2JqZWN0LT52bSgpOworI2lmIFBMQVRG T1JNKElPUykKKyAgICBzdGF0aWMgYm9vbCBuZWVkc09sZFN0cmluZ05hbWUgPSBpc1Bva2VyQnJv cygpOworICAgIGlmIChVTkxJS0VMWShuZWVkc09sZFN0cmluZ05hbWUpKSB7CisgICAgICAgIGNv bnN0IENsYXNzSW5mbyogaW5mbyA9IG9iamVjdC0+Y2xhc3NJbmZvKHZtKTsKKyAgICAgICAgQVNT RVJUKGluZm8pOworICAgICAgICByZXR1cm4gaW5mby0+Y2xhc3NOYW1lOworICAgIH0KKyNlbmRp ZgogICAgIGF1dG8gc2NvcGUgPSBERUNMQVJFX1RIUk9XX1NDT1BFKHZtKTsKICAgICBib29sIG9i amVjdElzQXJyYXkgPSBpc0FycmF5KGdsb2JhbE9iamVjdCwgb2JqZWN0KTsKICAgICBSRVRVUk5f SUZfRVhDRVBUSU9OKHNjb3BlLCBTdHJpbmcoKSk7Cg==