Bug 191638 - WebKit.WebsiteDataStoreCustomPaths API test is failing when enabling process prewarming
Summary: WebKit.WebsiteDataStoreCustomPaths API test is failing when enabling process ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 191634 191693
Blocks: 191572
  Show dependency treegraph
 
Reported: 2018-11-14 09:31 PST by Chris Dumez
Modified: 2018-11-15 08:50 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.54 KB, patch)
2018-11-14 09:48 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.68 KB, patch)
2018-11-14 15:38 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 2018-11-14 09:31:58 PST
WebKit.WebsiteDataStoreCustomPaths API test is failing when enabling process prewarming.
Comment 1 Chris Dumez 2018-11-14 09:48:33 PST
Created attachment 354824 [details]
Patch
Comment 2 Ryosuke Niwa 2018-11-14 14:21:10 PST
Comment on attachment 354824 [details]
Patch

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

> Tools/ChangeLog:13
> +        However, process prewarming constructs the default data store and passes it to the
> +        new WebProcessProxy if WebProcessPool::m_websiteDataStore is null.

Hm... wouldn't this affect existing apps that would pre-warm processes?
Comment 3 Chris Dumez 2018-11-14 14:39:39 PST
(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 354824 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354824&action=review
> 
> > Tools/ChangeLog:13
> > +        However, process prewarming constructs the default data store and passes it to the
> > +        new WebProcessProxy if WebProcessPool::m_websiteDataStore is null.
> 
> Hm... wouldn't this affect existing apps that would pre-warm processes?

What's the big deal about creating the default data store if it does not exist yet?
Comment 4 Brady Eidson 2018-11-14 15:13:37 PST
(In reply to Chris Dumez from comment #3)
> (In reply to Ryosuke Niwa from comment #2)
> > Comment on attachment 354824 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=354824&action=review
> > 
> > > Tools/ChangeLog:13
> > > +        However, process prewarming constructs the default data store and passes it to the
> > > +        new WebProcessProxy if WebProcessPool::m_websiteDataStore is null.
> > 
> > Hm... wouldn't this affect existing apps that would pre-warm processes?
> 
> What's the big deal about creating the default data store if it does not
> exist yet?

You can't do it because it's a huge no-no on iOS.
Comment 5 Chris Dumez 2018-11-14 15:38:05 PST
Created attachment 354862 [details]
Patch
Comment 6 Chris Dumez 2018-11-14 15:38:59 PST
Ok, I implemented an alternative approach which makes sure process prewarming tries to find a suitable data store but never creates the default one when it does not exist.
Comment 7 WebKit Commit Bot 2018-11-14 19:31:05 PST
Comment on attachment 354862 [details]
Patch

Clearing flags on attachment: 354862

Committed r238215: <https://trac.webkit.org/changeset/238215>
Comment 8 WebKit Commit Bot 2018-11-14 19:31:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-11-14 19:33:31 PST
<rdar://problem/46084619>
Comment 10 Truitt Savell 2018-11-15 08:23:26 PST
Looks like the changes in https://trac.webkit.org/changeset/238215/webkit

build:
https://trac.webkit.org/changeset/238215/webkit

has caused two API failures:

TestWebKitAPI.WKProcessPool.InitialWarmedProcessUsed
        
        /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessPreWarming.mm:62
        Value of: [pool _hasPrewarmedWebProcess]
          Actual: false
        Expected: true
        
        
        /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessPreWarming.mm:63
        Expected equality of these values:
          1U
            Which is: 1
          [pool _webPageContentProcessCount]
            Which is: 0
        

    TestWebKitAPI.WKProcessPool.WarmInitialProcess
        
        /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessPreWarming.mm:47
        Value of: [pool _hasPrewarmedWebProcess]
          Actual: false
        Expected: true
Comment 11 Chris Dumez 2018-11-15 08:35:01 PST
(In reply to Truitt Savell from comment #10)
> Looks like the changes in https://trac.webkit.org/changeset/238215/webkit
> 
> build:
> https://trac.webkit.org/changeset/238215/webkit
> 
> has caused two API failures:
> 
> TestWebKitAPI.WKProcessPool.InitialWarmedProcessUsed
>         
>        
> /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessPreWarming.mm:62
>         Value of: [pool _hasPrewarmedWebProcess]
>           Actual: false
>         Expected: true
>         
>         
>        
> /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessPreWarming.mm:63
>         Expected equality of these values:
>           1U
>             Which is: 1
>           [pool _webPageContentProcessCount]
>             Which is: 0
>         
> 
>     TestWebKitAPI.WKProcessPool.WarmInitialProcess
>         
>        
> /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessPreWarming.mm:47
>         Value of: [pool _hasPrewarmedWebProcess]
>           Actual: false
>         Expected: true

Will look now, thanks. I believe the test just needs updating.
Comment 12 Chris Dumez 2018-11-15 08:50:18 PST
(In reply to Chris Dumez from comment #11)
> (In reply to Truitt Savell from comment #10)
> > Looks like the changes in https://trac.webkit.org/changeset/238215/webkit
> > 
> > build:
> > https://trac.webkit.org/changeset/238215/webkit
> > 
> > has caused two API failures:
> > 
> > TestWebKitAPI.WKProcessPool.InitialWarmedProcessUsed
> >         
> >        
> > /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/ProcessPreWarming.mm:62
> >         Value of: [pool _hasPrewarmedWebProcess]
> >           Actual: false
> >         Expected: true
> >         
> >         
> >        
> > /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/ProcessPreWarming.mm:63
> >         Expected equality of these values:
> >           1U
> >             Which is: 1
> >           [pool _webPageContentProcessCount]
> >             Which is: 0
> >         
> > 
> >     TestWebKitAPI.WKProcessPool.WarmInitialProcess
> >         
> >        
> > /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/ProcessPreWarming.mm:47
> >         Value of: [pool _hasPrewarmedWebProcess]
> >           Actual: false
> >         Expected: true
> 
> Will look now, thanks. I believe the test just needs updating.

https://bugs.webkit.org/show_bug.cgi?id=191693 Patch coming shortly.