| Summary: | REGRESSION (r231107): MoviStar+ launches to a blank black screen | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Daniel Bates <dbates> |
| Component: | WebCore Misc. | Assignee: | Alex Christensen <achristensen> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | achristensen, benjamin, bfulgham, cdumez, cmarcelo, ews-watchlist, japhet, mitz, rniwa, webkit-bug-importer, youennf |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Local Build | ||
| Hardware: | iPhone / iPad | ||
| OS: | Unspecified | ||
| Bug Depends on: | 184741 | ||
| Bug Blocks: | |||
| Attachments: | |||
Created attachment 346095 [details]
Patch
Comment on attachment 346095 [details]
Patch
r=me.
Comment on attachment 346095 [details] Patch Attachment 346095 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8703124 New failing tests: imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.worker.html Created attachment 346104 [details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 346105 [details]
Patch and layout test
Added a layout test to ensure that setting is propagated to both the WebKit1 and WebKit2 code paths. Ideally we would use a TestWebKitAPI test that registers a custom protocol handler and loads content that makes a CORS preflight request to its custom protocol.
Comment on attachment 346105 [details] Patch and layout test Attachment 346105 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8703907 New failing tests: imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.worker.html Created attachment 346116 [details]
Archive of layout-test-results from ews100 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 346119 [details] Patch and layout tests Add platform-specific results for OS X Yosemite, macOS Sierra, macOS High Sierra and iOS 11 that use the CORS behavior before <http://trac.webkit.org/changeset/231107>. Comment on attachment 346119 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=346119&action=review I think this patch looks good, but it might still cause imported platform Fetch API tests to fail. > Source/WebKit/NetworkProcess/NetworkLoadChecker.h:137 > + bool m_shouldCaptureExtraNetworkLoadMetrics; We did you stop initializing this value? Is it because it is always set by the constructor? It seems like it would be better to always set them to a known state, in case someone makes a constructor overload in the future that doesn't assign a value here. Comment on attachment 346119 [details] Patch and layout tests Attachment 346119 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8705153 New failing tests: http/tests/xmlhttprequest/about-put-allowCORSPreflightRequestsToAnyScheme.html Created attachment 346131 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 346119 [details] Patch and layout tests Attachment 346119 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8705094 New failing tests: imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.worker.html Created attachment 346132 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 346119 [details] Patch and layout tests Attachment 346119 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8706419 New failing tests: imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.worker.html Created attachment 346141 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 346119 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=346119&action=review > LayoutTests/ChangeLog:26 > + * platform/mac-yosemite/imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any-expected.txt: Added. > + * platform/mac-yosemite/imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.worker-expected.txt: Added. These aren't needed after http://trac.webkit.org/r234435 Comment on attachment 346119 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=346119&action=review > Source/WebCore/page/Settings.yaml:763 > +allowCORSPreflightRequestsToAnyScheme: > + initial: false I'll bet this is being reset to false every time a test runs, leading to the test failures if the affected tests aren't the first run by DRT/WKTR https://trac.webkit.org/changeset/219847/webkit might be related to the failures were seeing. I think we should add a bundle check, too, so we can remove this hack soon. Created attachment 346247 [details]
Patch
Comment on attachment 346247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346247&action=review > Source/WebCore/loader/DocumentThreadableLoader.cpp:188 > + bool needsPreflightQuirk = IOSApplication::isMoviStarPlus() && applicationSDKVersion() < DYLD_IOS_VERSION_12_0 && (m_options.preflightPolicy == PreflightPolicy::Consider || m_options.preflightPolicy == PreflightPolicy::Force); How did you come to the decision to limit this change only to MoviStar? This issue affects all apps. It seems weird to give preferential treatment to MoviStar and will give a bad customer experience for other Mac and iOS apps affected by r231107. Comment on attachment 346247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346247&action=review Let’s take this change as is, and revisit for other impacted client applications if any are identified, >> Source/WebCore/loader/DocumentThreadableLoader.cpp:188 >> + bool needsPreflightQuirk = IOSApplication::isMoviStarPlus() && applicationSDKVersion() < DYLD_IOS_VERSION_12_0 && (m_options.preflightPolicy == PreflightPolicy::Consider || m_options.preflightPolicy == PreflightPolicy::Force); > > How did you come to the decision to limit this change only to MoviStar? This issue affects all apps. It seems weird to give preferential treatment to MoviStar and will give a bad customer experience for other Mac and iOS apps affected by r231107. Do we know of any other clients impacted by this? Alex and I talked about it, and would like to be able to remove this workaround code once MoviStar (and any others) have updated. Created attachment 346255 [details]
Patch
(In reply to Daniel Bates from comment #22) > Comment on attachment 346247 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346247&action=review > > > Source/WebCore/loader/DocumentThreadableLoader.cpp:188 > > + bool needsPreflightQuirk = IOSApplication::isMoviStarPlus() && applicationSDKVersion() < DYLD_IOS_VERSION_12_0 && (m_options.preflightPolicy == PreflightPolicy::Consider || m_options.preflightPolicy == PreflightPolicy::Force); > > How did you come to the decision to limit this change only to MoviStar? This > issue affects all apps. It seems weird to give preferential treatment to > MoviStar and will give a bad customer experience for other Mac and iOS apps > affected by r231107. I agree with Dan. It’s not clear how r231107 benefits folks who use apps linked before iOS 12, but it’s clear that r231107 carries risk to those people. (In reply to mitz from comment #26) I think this is measurably better than nothing. Either of the Dans are welcome to improve upon this by the integration deadline. |
Steps to reproduce: 1. Download and install MoviStar+ from the Spain App Store. To do this from the US you will need a billing address in Spain and need to perform the following: A. Open Settings > General > Language & Region. Tap Region and pick Spain. B. Visit <https://appleid.apple.com/account#!&page=create> and create a new Apple ID and choose Spain as your country. C. Open Settings > iTunes & App Stores and sign in with the Apple ID you created in (B). If prompted to enter payment details and billing address select None and enter your billing address, respectively. D. Open the App Store. 2. Open MoviStar+. Then after the initial loading screen you will see a blank black screen.