REGRESSION (r259531?): [iOS] TestWebKitAPI.WebKitLegacy.ScrollingDoesNotPauseMedia is timing out

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

TestWebKitAPI.WebKitLegacy.ScrollingDoesNotPauseMedia seems to be failing consistently.

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

Results database indicate regression point as r259531 which was today (April 4). However, this issue was first noticed on EWS 2 days back (April 2) in https://ews-build.webkit.org/#/builders/9/builds/21024

[Radar WebKit Bug Importer]

<rdar://problem/61353942>

[Peng Liu]

This seems to be problem for a while. See comment 14 of webkit.org/b/192829.

[Ryan Haddad]

(In reply to Peng Liu from comment #2)
> This seems to be problem for a while. See comment 14 of webkit.org/b/192829.
It looks like the test has been intermittently timing out for a while, but something changed with r259531 that made it 100% reproducible.

[Peng Liu]

Created attachment 395634 [details]
Patch

[Daniel Bates]

Comment on attachment 395634 [details]
Patch

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

Patch looks good.

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:41
> +static bool didFinishLoad = false;

Ok as is. No change is needed.  The current code is the optimal solution: no initialization is needed.

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:42
> +static bool gotMainFrame = false;

Ditto

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:45
> +static bool readyToTest = false;

Ditto for this line and 2 more below.

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:46
> +static bool didReceivePause  = false;

Ok as is. No change needed. Look like extra space after before =

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:79
> +    RetainPtr<WebPreferences> preferences = [WebPreferences standardPreferences];

Ok as is. No change needed. The optimal solution stores into raw pointer because function returns process global singleton.

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:80
> +    preferences.get().mediaDataLoadsAutomatically = YES;

Ok as is. No change needed. The optimal solution is to not use .get() and switch to [preferences serX:....] notation because it aesthetically  more pleasing to read and idiomatic. Same remarks for next two lines below

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:91
> +    [uiWebView loadRequest:[NSURLRequest requestWithURL:[NSBundle.mainBundle URLForResource:@"one-video" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];

Ok as is. No change needed. There may be an existing helper method that returns the resource URL relative to the bundle or a test function that does that + load it (at least there is one on TestWKWebView$.

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:99
> +        DOMHTMLMediaElement* video = (DOMHTMLMediaElement*)[[mainFrame DOMDocument] querySelector:@"video"];

Ok as is. No change needed. The optimal solution uses RetainPtr because future proofs this code should in the future is added that causes document destruction.

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:113
> +        DOMHTMLMediaElement* video = (DOMHTMLMediaElement*)[[mainFrame DOMDocument] querySelector:@"video"];

Ok as is. No change needed. The optimal  solution uses retainptr

[EWS]

Committed r259622: <https://trac.webkit.org/changeset/259622>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395634 [details].

[Daniel Bates]

Comment on attachment 395634 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:99
>> +        DOMHTMLMediaElement* video = (DOMHTMLMediaElement*)[[mainFrame DOMDocument] querySelector:@"video"];
> 
> Ok as is. No change needed. The optimal solution uses RetainPtr because future proofs this code should in the future is added that causes document destruction.

Optimal solution is right, but the reason for it is wrong (since the DOMNode refs the WebCore::Node). The real reason is to future proof against future code that may drain the autorelease pool.