Bug 215293 – REGRESSION(r261159) PokerBros only shows black screen

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

REGRESSION(r261159) PokerBros only shows black screen

[Alex Christensen]

Created attachment 406216 [details]
Patch

[Keith Miller]

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.

[Alex Christensen]

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.

[Alex Christensen]

* 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

[Radar WebKit Bug Importer]

<rdar://problem/66702248>

[Darin Adler]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 215316

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 406300 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r265431

[Simon Fraser (smfr)]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Alex Christensen]

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

[Darin Adler]

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

[Alex Christensen]

(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