Bug 184893 - [iOS] API test ScrollViewInsetTests.InnerHeightWithLargeTopContentInset is a flaky failure
Summary: [iOS] API test ScrollViewInsetTests.InnerHeightWithLargeTopContentInset is a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-23 11:24 PDT by Ryan Haddad
Modified: 2018-05-31 09:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.15 KB, patch)
2018-05-30 16:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.12 KB, patch)
2018-05-31 09:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.