REGRESSION(r261159) PokerBros only shows black screen
Created attachment 406216 [details] Patch
Comment on attachment 406216 [details] 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.
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.
* 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
<rdar://problem/66702248>
Comment on attachment 406216 [details] 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.
Re-opened since this is blocked by bug 215316
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.
Created attachment 406300 [details] Patch
http://trac.webkit.org/r265431
Comment on attachment 406300 [details] 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().
Comment on attachment 406300 [details] 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.
Comment on attachment 406300 [details] 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.
"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.
(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.
(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