REGRESSION: ( r254222 ) [ Mac Debug ] TestWebKitAPI.AsyncFunction.Promise is timing out

[Truitt Savell]

TestWebKitAPI.AsyncFunction.Promise

Description:
This API test was introduced as timing out from https://trac.webkit.org/changeset/254222/webkit

History:
https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.AsyncFunction.Promise

[Radar WebKit Bug Importer]

<rdar://problem/58445164>

[Jonathan Bedard]

This is specific to debug builds.

[Brady Eidson]

Created attachment 388055 [details]
Patch

[Brady Eidson]

Want a review on this from a JSC guy - This forced GC pass with only 1 function call appears to work! Should I actually expect that it works?

[Brady Eidson]

Created attachment 388059 [details]
Patch

[Keith Miller]

Comment on attachment 388059 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388059&action=review

r- because I think this is backwards. But maybe I'm misunderstanding the logic.

> Tools/ChangeLog:9
> +        But it turns out ne copy of the function call and an API call to force a GC pass seems to be enough.

Nit: Typo, I'm not sure what the exact sentence you were trying to say was though... :P

> Tools/TestWebKitAPI/Tests/WebKitCocoa/AsyncFunction.mm:275
> +        [webView _callAsyncJavaScriptFunction:functionBody withArguments:nil inWorld:_WKContentWorld.pageContentWorld completionHandler:[&] (id result, NSError *error) {
> +            EXPECT_NULL(result);
> +            EXPECT_TRUE(error != nil);
> +            EXPECT_TRUE([[error description] containsString:@"no longer reachable"]);
> +            done = true;

I think this is the opposite of what you want? I think you want to confirm that it returned an error at least once not 100% of the time.

[Brady Eidson]

(In reply to Keith Miller from comment #6)
> Comment on attachment 388059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388059&action=review
> 
> r- because I think this is backwards. But maybe I'm misunderstanding the
> logic.
> 
> > Tools/ChangeLog:9
> > +        But it turns out ne copy of the function call and an API call to force a GC pass seems to be enough.
> 
> Nit: Typo, I'm not sure what the exact sentence you were trying to say was
> though... :P
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/AsyncFunction.mm:275
> > +        [webView _callAsyncJavaScriptFunction:functionBody withArguments:nil inWorld:_WKContentWorld.pageContentWorld completionHandler:[&] (id result, NSError *error) {
> > +            EXPECT_NULL(result);
> > +            EXPECT_TRUE(error != nil);
> > +            EXPECT_TRUE([[error description] containsString:@"no longer reachable"]);
> > +            done = true;
> 
> I think this is the opposite of what you want? I think you want to confirm
> that it returned an error at least once not 100% of the time.

Actually all this does is checks that *at least one* returns an error.
The promise *never* gets resolved, so the callback will never take place... UNLESS it takes place because of GC.

[Keith Miller]

> > I think this is the opposite of what you want? I think you want to confirm
> > that it returned an error at least once not 100% of the time.
> 
> Actually all this does is checks that *at least one* returns an error.
> The promise *never* gets resolved, so the callback will never take place...
> UNLESS it takes place because of GC.

Ah, misread the code. r=me with the nit in the ChangeLog then.

[Brady Eidson]

Created attachment 388069 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 388069 [details]
Patch

Clearing flags on attachment: 388069

Committed r254756: <https://trac.webkit.org/changeset/254756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.