Summary: | Add error information to help debug test failure in WKNavigation.ProcessCrashDuringCallback | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||||
Component: | Tools / Tests | Assignee: | Sihui Liu <sihui_liu> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, jlewis3, lforschler, realdawei, ryanhaddad, tsavell, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 189148 | ||||||||||||
Attachments: |
|
Description
Sihui Liu
2018-08-27 17:15:29 PDT
Created attachment 348242 [details]
Patch
Created attachment 348304 [details]
Patch
What does the failure look like? (In reply to Chris Dumez from comment #3) > What does the failure look like? https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/4651/steps/run-api-tests/logs/stdio /Volumes/Data/slave/highsierra-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm:232 Value of: !!error Actual: false Expected: true Comment on attachment 348304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348304&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm:233 > + EXPECT_EQ(error.code, WKErrorWebContentProcessTerminated); I believe the parameters are reversed. Left side should be expected value iirc. Created attachment 348320 [details]
Patch
(In reply to Chris Dumez from comment #5) > Comment on attachment 348304 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348304&action=review > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm:233 > > + EXPECT_EQ(error.code, WKErrorWebContentProcessTerminated); > > I believe the parameters are reversed. Left side should be expected value > iirc. I saw this WKContentExtensionStoreStore.mm so I thought it's correct... Fixed. (In reply to Sihui Liu from comment #7) > (In reply to Chris Dumez from comment #5) > > Comment on attachment 348304 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=348304&action=review > > > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebContentProcessDidTerminate.mm:233 > > > + EXPECT_EQ(error.code, WKErrorWebContentProcessTerminated); > > > > I believe the parameters are reversed. Left side should be expected value > > iirc. > > I saw this WKContentExtensionStoreStore.mm so I thought it's correct... > Fixed. Yes, people get it wrong all the time. Comment on attachment 348320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348320&action=review r=me > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4576 > + NSError* callbackError = [NSError errorWithDomain:WKErrorDomain code:static_cast<int>(error) userInfo:nil]; FYI, we use star on right size for NS types. But in this case, I'd rather we drop this local variable completely: completionHandlerBlock(nil, [NSError errorWithDomain:WKErrorDomain code:static_cast<int>(error) userInfo:nil]); > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4591 > + NSError* callbackError = [NSError errorWithDomain:WKErrorDomain code:static_cast<int>(error) userInfo:nil]; ditto. > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4605 > + NSError* callbackError = [NSError errorWithDomain:WKErrorDomain code:static_cast<int>(error) userInfo:nil]; ditto. Created attachment 348338 [details]
Patch for landing
Comment on attachment 348338 [details] Patch for landing Clearing flags on attachment: 348338 Committed r235440: <https://trac.webkit.org/changeset/235440> All reviewed patches have been landed. Closing bug. Do we now have the needed information to resolve the issue? This test is consistently failing on iOS bots. https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/7110/steps/run-api-tests/logs/stdio (In reply to Ryan Haddad from comment #14) > Do we now have the needed information to resolve the issue? This test is > consistently failing on iOS bots. > > https://build.webkit.org/builders/ > Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/7110/steps/ > run-api-tests/logs/stdio Probably the WebView is getting destroyed *before* we realize the WebProcess has exited. When The WebView gets destroyed, we call WebPageProxy::close() which causes error WKErrorWebViewInvalidated as we see in the new test output. (In reply to Chris Dumez from comment #15) > (In reply to Ryan Haddad from comment #14) > > Do we now have the needed information to resolve the issue? This test is > > consistently failing on iOS bots. > > > > https://build.webkit.org/builders/ > > Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/7110/steps/ > > run-api-tests/logs/stdio > > Probably the WebView is getting destroyed *before* we realize the WebProcess > has exited. When The WebView gets destroyed, we call WebPageProxy::close() > which causes error WKErrorWebViewInvalidated as we see in the new test > output. We should probably drop this line: webView = nullptr; Not sure why I did this in the first place. |