Summary: | Lion Intel Debug WebKit2 Tests crashing under [WKFullScreenWindowController _startEnterFullScreenAnimationWithDuration:] | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jessie Berlin <jberlin> | ||||||||
Component: | WebKit2 | Assignee: | Jer Noble <jer.noble> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, jberlin, jer.noble, thorton, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar, LayoutTestFailure, MakingBotsRed, Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
URL: | http://build.webkit.org/results/Lion%20Intel%20Debug%20(WebKit2%20Tests)/r110602%20(4860)/fullscreen/anonymous-block-merge-crash-crash-log.txt | ||||||||||
Attachments: |
|
Description
Jessie Berlin
2012-03-13 16:33:43 PDT
InjectedBundlePageFullScreenClient needs to be taught about beganEnterFullScreen and beganExitFullScreen, so that WebKitTestRunner can intercept these calls and not attempt to move around windows, etc. Created attachment 131756 [details]
Patch
Comment on attachment 131756 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131756&action=review > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:322 > + > + // Version 1: > + WKBundlePageBeganEnterFullScreen beganEnterFullScreen; > + WKBundlePageBeganExitFullScreen beganExitFullScreen; > }; > typedef struct WKBundlePageFullScreenClient WKBundlePageFullScreenClient; > > -enum { kWKBundlePageFullScreenClientCurrentVersion = 0 }; > +enum { kWKBundlePageFullScreenClientCurrentVersion = 1 }; You need to add an APIClientTraits specialization for WKBundlePageFullScreenClient now, so that the struct is properly zeroed out for version 0 clients. Created attachment 131767 [details]
Patch
Comment on attachment 131767 [details] Patch Attachment 131767 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11948832 Created attachment 131860 [details]
Patch
Fixed windows build failure.
Comment on attachment 131860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131860&action=review r=me! > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFullScreenClient.cpp:77 > + m_client.beganExitFullScreen(toAPI(page), toAPI(initialFrame), toAPI(finalFrame)); I think you meant m_client.beganEnterFullScreen( .... here, not beganExitFullScreen. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1118 > +void InjectedBundlePage::beganEnterFullScreen(WKBundlePageRef pageRef, WKRect initialFrame, WKRect finalFrame) Odd that you aren't seeing any compiler warnings / errors about unused parameters. You should remove the names for the parameters here. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1124 > +void InjectedBundlePage::beganExitFullScreen(WKBundlePageRef pageRef, WKRect initialFrame, WKRect finalFrame) Ditto. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1127 > + InjectedBundle::shared().os() << "beganExitFullScreen()\n"; Do you know if there are any tests that depend on beganEnterFullScreen/beganExitFullScreen being in the output? If so, are there expected failing results checked in for them for WK2 that would need to be updated now that we are logging it? Comment on attachment 131860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131860&action=review >> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFullScreenClient.cpp:77 >> + m_client.beganExitFullScreen(toAPI(page), toAPI(initialFrame), toAPI(finalFrame)); > > I think you meant > > m_client.beganEnterFullScreen( .... > > here, not beganExitFullScreen. Whoops! Changed. >> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1118 >> +void InjectedBundlePage::beganEnterFullScreen(WKBundlePageRef pageRef, WKRect initialFrame, WKRect finalFrame) > > Odd that you aren't seeing any compiler warnings / errors about unused parameters. You should remove the names for the parameters here. Odd indeed. Odder still that I thought I removed them. Though the other functions in the file have the same problem. I'll remove them anyways. >> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1127 >> + InjectedBundle::shared().os() << "beganExitFullScreen()\n"; > > Do you know if there are any tests that depend on beganEnterFullScreen/beganExitFullScreen being in the output? If so, are there expected failing results checked in for them for WK2 that would need to be updated now that we are logging it? No, none of the tests seem to enable dumping. And, while there are a couple of failing tests when running "run-webkit-tests -2", none are due to logging. Committed r110708: <http://trac.webkit.org/changeset/110708> > Odd indeed. Odder still that I thought I removed them. Though the other functions in the file have the same problem. I'll remove them anyways.
Sam once told me that some of WebKit2 code has argument names left intentionally. Not sure if that's one of those cases.
(In reply to comment #11) > > Odd indeed. Odder still that I thought I removed them. Though the other functions in the file have the same problem. I'll remove them anyways. > > Sam once told me that some of WebKit2 code has argument names left intentionally. Not sure if that's one of those cases. Maybe he was referring to leaving the argument names in the WK API header files, even if they are obvious? He has told me that a couple of times. |