Bug 189037 – Add error information to help debug test failure in WKNavigation.ProcessCrashDuringCallback

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Sihui Liu]

WKNavigation.ProcessCrashDuringCallback is failing after r235321, which seems to be an irrelevant change. Add some error information to help debug the failure.

[Sihui Liu]

Created attachment 348242 [details]
Patch

[Sihui Liu]

Created attachment 348304 [details]
Patch

[Chris Dumez]

What does the failure look like?

[Sihui Liu]

(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

[Chris Dumez]

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.

[Sihui Liu]

Created attachment 348320 [details]
Patch

[Sihui Liu]

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

[Chris Dumez]

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

[Chris Dumez]

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.

[Sihui Liu]

Created attachment 348338 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 348338 [details]
Patch for landing

Clearing flags on attachment: 348338

Committed r235440: <https://trac.webkit.org/changeset/235440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43814405>

[Ryan Haddad]

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

[Chris Dumez]

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

[Chris Dumez]

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