Bug 194829 - [WKTR] Avoid starting new NetworkProcesses unnecessarily when running the layout tests
Summary: [WKTR] Avoid starting new NetworkProcesses unnecessarily when running the lay...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-19 14:03 PST by Chris Dumez
Modified: 2019-02-21 13:54 PST (History)
11 users (show)

See Also:


Attachments
Patch (17.52 KB, patch)
2019-02-19 14:19 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (88.80 MB, application/zip)
2019-02-19 16:43 PST, EWS Watchlist
no flags Details
Patch (17.54 KB, patch)
2019-02-20 08:56 PST, 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 Chris Dumez 2019-02-19 14:03:01 PST
Avoid starting new NetworkProcesses unnecessarily when running the layout tests.
Comment 1 Chris Dumez 2019-02-19 14:03:19 PST
<rdar://problem/47889906>
Comment 2 Chris Dumez 2019-02-19 14:19:22 PST
Created attachment 362427 [details]
Patch
Comment 3 Jonathan Bedard 2019-02-19 15:21:27 PST
High-level: This is a good idea, probably has efficiency wins beyond just de-duplicating processes.

That being said, I'm a bit concerned that this is how we solved the problem. If the WKContext leak was in the test harness, then fair enough, but I get the sense that the WKContext leak is a general WebKit problem. Creating a new WKContext and deleting the old one is a supported workflow, right?
Comment 4 Chris Dumez 2019-02-19 15:30:42 PST
(In reply to Jonathan Bedard from comment #3)
> High-level: This is a good idea, probably has efficiency wins beyond just
> de-duplicating processes.
> 
> That being said, I'm a bit concerned that this is how we solved the problem.
> If the WKContext leak was in the test harness, then fair enough, but I get
> the sense that the WKContext leak is a general WebKit problem. Creating a
> new WKContext and deleting the old one is a supported workflow, right?

Only 1 WKContext is leaking (the first one). This patch does not fix that and I will investigate it too. This patch does not prevent us from seeing or fixing the leak of the first WKContext.
Comment 5 Alexey Proskuryakov 2019-02-19 15:40:00 PST
Comment on attachment 362427 [details]
Patch

r=me pending EWS
Comment 6 EWS Watchlist 2019-02-19 16:43:20 PST
Comment on attachment 362427 [details]
Patch

Attachment 362427 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11209559

New failing tests:
fast/dom/Geolocation/floorLevel.html
Comment 7 EWS Watchlist 2019-02-19 16:43:28 PST
Created attachment 362453 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 8 Chris Dumez 2019-02-20 08:56:10 PST
Created attachment 362499 [details]
Patch
Comment 9 WebKit Commit Bot 2019-02-20 10:34:51 PST
Comment on attachment 362499 [details]
Patch

Clearing flags on attachment: 362499

Committed r241821: <https://trac.webkit.org/changeset/241821>
Comment 10 WebKit Commit Bot 2019-02-20 10:34:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-02-20 10:43:14 PST
<rdar://problem/48242710>
Comment 12 Truitt Savell 2019-02-20 17:24:25 PST
Interesting failure caused by https://trac.webkit.org/changeset/241821/webkit

fast/mediastream/MediaStream-video-element.html has become a constant failure on Mac WK2. the failure only reproduces when running this test with the test that runs right before it in the test runner. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fmediastream%2FMediaStream-video-element.html

repro:

run-webkit-tests --root testbuild-241821 fast/mediastream/MediaStream-video-element-video-tracks-disabled.html fast/mediastream/MediaStream-video-element.html --child-processes 1

the test fails on r241821 100% but passes on 241820.
Comment 13 Truitt Savell 2019-02-20 17:26:37 PST
fast/mediastream/MediaStream-video-element.html Diff:

--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/fast/mediastream/MediaStream-video-element-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/fast/mediastream/MediaStream-video-element-actual.txt
@@ -47,7 +47,7 @@
 video.audioTracks[0] properties:
   track.id = <UUID>
   track.kind = main
-  track.label = Mock audio device 1
+  track.label = 
   track.language = 
   track.enabled = true
   track.sourceBuffer = null
Comment 14 Jonathan Bedard 2019-02-20 17:44:57 PST
(In reply to Truitt Savell from comment #13)
> fast/mediastream/MediaStream-video-element.html Diff:
> 
> ---
> /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> fast/mediastream/MediaStream-video-element-expected.txt
> +++
> /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> fast/mediastream/MediaStream-video-element-actual.txt
> @@ -47,7 +47,7 @@
>  video.audioTracks[0] properties:
>    track.id = <UUID>
>    track.kind = main
> -  track.label = Mock audio device 1
> +  track.label = 
>    track.language = 
>    track.enabled = true
>    track.sourceBuffer = null

I am of the opinion that this test failure means that this patch exposed another underlying WebKit bug, but did not cause this bug.
Comment 15 Chris Dumez 2019-02-20 19:59:18 PST
(In reply to Jonathan Bedard from comment #14)
> (In reply to Truitt Savell from comment #13)
> > fast/mediastream/MediaStream-video-element.html Diff:
> > 
> > ---
> > /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> > fast/mediastream/MediaStream-video-element-expected.txt
> > +++
> > /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> > fast/mediastream/MediaStream-video-element-actual.txt
> > @@ -47,7 +47,7 @@
> >  video.audioTracks[0] properties:
> >    track.id = <UUID>
> >    track.kind = main
> > -  track.label = Mock audio device 1
> > +  track.label = 
> >    track.language = 
> >    track.enabled = true
> >    track.sourceBuffer = null
> 
> I am of the opinion that this test failure means that this patch exposed
> another underlying WebKit bug, but did not cause this bug.

Agreed.


fast/mediastream/MediaStream-video-element-video-tracks-disabled.html usesenableModernMediaControls=false so it would have previously caused us to recreate a WebView and a WKContext. After my change, we now reuse the context and merely reconstruct a new WebView. This is somehow causing this flakiness.
Comment 16 Alexey Proskuryakov 2019-02-21 09:36:59 PST
Is there a new bug tracking the failure?
Comment 17 Chris Dumez 2019-02-21 13:54:43 PST
(In reply to Alexey Proskuryakov from comment #16)
> Is there a new bug tracking the failure?

https://bugs.webkit.org/show_bug.cgi?id=194916