Bug 184893

Summary: [iOS] API test ScrollViewInsetTests.InnerHeightWithLargeTopContentInset is a flaky failure
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, commit-queue, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=183648
Attachments:
Description Flags
Patch
none
Patch none

Description Ryan Haddad 2018-04-23 11:24:05 PDT
API test ScrollViewInsetTests.InnerHeightWithLargeTopContentInset is a flaky failure on iOS Simulator

FAIL ScrollViewInsetTests.InnerHeightWithLargeTopContentInset

/Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:86
Value of: [[webView objectByEvaluatingJavaScript:@"innerHeight"] floatValue]
  Actual: 100
Expected: viewHeight
Which is: 500

https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/4516
Comment 1 Wenson Hsieh 2018-04-23 19:54:01 PDT
On our internal iOS bots (those of which are not on iOS 11.3), this test seems to be passing all the time. In the past 2 weeks, this test was run on that build train 500+ times with no failures.

I wonder if something's different about our OpenSource bots...I'll try running these tests with an iOS 11.3 SDK and see if I can reproduce.
Comment 2 Radar WebKit Bug Importer 2018-04-25 22:02:58 PDT
<rdar://problem/39747271>
Comment 3 Chris Dumez 2018-05-30 16:52:23 PDT
Created attachment 341623 [details]
Patch
Comment 4 Wenson Hsieh 2018-05-30 19:29:27 PDT
Comment on attachment 341623 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:81
> +    int tries = 0;

Do we really need a max number of tries here? I think letting it just time out isn't a bad thing, per se...

Also, perhaps this would just work if we waited until the next *visible content rect* update instead of the next remote layer tree commit, since visible content rect update is the mechanism by which we account for scroll view content insets. It's possible we don't even need the "wait-until-done" loop! There's WKWebView SPI to do this: see -_doAfterNextVisibleContentRectUpdate:.
Comment 5 Chris Dumez 2018-05-30 20:14:21 PDT
(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 341623 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341623&action=review
> 
> > Tools/TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:81
> > +    int tries = 0;
> 
> Do we really need a max number of tries here? I think letting it just time
> out isn't a bad thing, per se...
> 

I have been told the opposite several times. We prefer quick failures over time outs.

> Also, perhaps this would just work if we waited until the next *visible
> content rect* update instead of the next remote layer tree commit, since
> visible content rect update is the mechanism by which we account for scroll
> view content insets. It's possible we don't even need the "wait-until-done"
> loop! There's WKWebView SPI to do this: see
> -_doAfterNextVisibleContentRectUpdate:.

Ok i will try tomorrow.
Comment 6 Chris Dumez 2018-05-31 08:45:40 PDT
(In reply to Chris Dumez from comment #5)
> (In reply to Wenson Hsieh from comment #4)
> > Comment on attachment 341623 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=341623&action=review
> > 
> > > Tools/TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:81
> > > +    int tries = 0;
> > 
> > Do we really need a max number of tries here? I think letting it just time
> > out isn't a bad thing, per se...
> > 
> 
> I have been told the opposite several times. We prefer quick failures over
> time outs.
> 
> > Also, perhaps this would just work if we waited until the next *visible
> > content rect* update instead of the next remote layer tree commit, since
> > visible content rect update is the mechanism by which we account for scroll
> > view content insets. It's possible we don't even need the "wait-until-done"
> > loop! There's WKWebView SPI to do this: see
> > -_doAfterNextVisibleContentRectUpdate:.
> 
> Ok i will try tomorrow.

_doAfterNextVisibleContentRectUpdate does not fix the flakiness.
Comment 7 Chris Dumez 2018-05-31 09:09:03 PDT
Created attachment 341664 [details]
Patch
Comment 8 WebKit Commit Bot 2018-05-31 09:47:38 PDT
Comment on attachment 341664 [details]
Patch

Clearing flags on attachment: 341664

Committed r232351: <https://trac.webkit.org/changeset/232351>
Comment 9 WebKit Commit Bot 2018-05-31 09:47:39 PDT
All reviewed patches have been landed.  Closing bug.