Bug 234933

Summary: [AppleWin] Some fast/shadow-dom/fullscreen-in-* tests are crashing after r287698
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ews-watchlist, jenner, keith_miller, mark.lam, mjs, msaboff, pvollan, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=233963
Bug Depends on:    
Bug Blocks: 233963    
Attachments:
Description Flags
Patch none

Description Fujii Hironori 2022-01-06 13:23:41 PST
Since r287698 (Bug 233963), AppleWin is crashing.

Regressions: Unexpected crashes (5)
  accessibility/insert-children-assert.html [ Crash ]
  fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html [ Crash ]
  fast/shadow-dom/fullscreen-in-shadow-full-screen-ancestor.html [ Crash ]
  fast/shadow-dom/fullscreen-in-shadow-fullscreenElement.html [ Crash ]
  fast/shadow-dom/fullscreen-in-slot-fullscreenElement.html [ Crash ]
Comment 1 Fujii Hironori 2022-01-06 13:29:10 PST
Since WinCairo skip fast/shadow-dom tests, I mannually skipped those tests and ran tests locally. I don't observe such crash with WinCairo.
I thinik WinCairo bots and I are using newer version of MSVC than AppleWin EWS bots. It's worth to try updating the compiler before debugging.
Comment 2 Fujii Hironori 2022-01-07 17:30:59 PST
r287770 reverted the change. The crashes have gone.
> 23:09:08.281 35661 worker/7 fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html passed
Comment 3 Alex Christensen 2022-01-08 08:48:24 PST
There is a significant amount of different code between AppleWin and WinCairo in the fullscreen implementation.  Could someone check running AppleWin on those tests with bug 233963 applied to see whether it is just needing newer MSVC or whether there is something going on in that code?
Comment 4 Fujii Hironori 2022-01-08 19:21:08 PST
I'm using Visual Studio 2019 version 16.11.8.
I have to work around some compilation errors to compile out AppleWin. https://gist.github.com/fujii/6dd1d7da709ca7c9015b1d0b82b289d9
I don't observe such crashes for AppleWin on my env.
Comment 6 Robert Jenner 2022-01-12 10:39:41 PST
This was also slowing down EWS on Windows. I have added a test expectation for these tests as [ Crash ] for Windows, to take care of the lag on EWS while this is being investigated. Test expectations were added here:

https://trac.webkit.org/changeset/287932/webkit
Comment 7 Robert Jenner 2022-01-12 10:45:27 PST
I missed marking the accessibility test that goes along with this bug as well. Marked here:

https://trac.webkit.org/changeset/287933/webkit
Comment 8 Radar WebKit Bug Importer 2022-01-13 13:24:16 PST
<rdar://problem/87567514>
Comment 9 Alex Christensen 2022-01-20 18:40:24 PST
Fujii, could you verify which version of MSVC you are using that does not exhibit these crashes?  https://build.webkit.org/#/builders/56/builds/8455/steps/9/logs/stdio line 87 says this from our open source builders:
"The CXX compiler identification is MSVC 19.23.28106.4"

Our internal builders are using 19.24.28316.0 and I'm wondering if that's new enough.
Comment 10 Fujii Hironori 2022-01-20 23:09:47 PST
They are Visual Studio 2019 Version 16.3 and 16.4.
https://en.wikipedia.org/wiki/Microsoft_Visual_C%2B%2B

I'm using version 16.11 that is the best for C++20.
https://devblogs.microsoft.com/cppblog/msvc-cpp20-and-the-std-cpp20-switch/
Comment 11 Alex Christensen 2022-01-20 23:34:52 PST
If it wouldn't be too much trouble, could you do a clean build and look for "The CXX compiler identification is MSVC xx.xx.xxxxx.x" and let me know what those exact numbers are?  That's all I have to compare from the logs.
Comment 12 Alex Christensen 2022-01-21 00:14:31 PST
I verified all your changes were necessary and I left the m_download = nullptr; at the end and am committing it in https://bugs.webkit.org/show_bug.cgi?id=235431
Comment 13 Alex Christensen 2022-01-21 16:44:02 PST
Created attachment 449711 [details]
Patch
Comment 14 Maciej Stachowiak 2022-01-22 13:43:59 PST
Comment on attachment 449711 [details]
Patch

r=me
Comment 15 Alex Christensen 2022-01-24 14:03:26 PST
Thanks for the review, but all the blocking radars need to be resolved before this can land.
Comment 16 EWS 2022-03-01 10:23:55 PST
Committed r290656 (247929@main): <https://commits.webkit.org/247929@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449711 [details].