Bug 196149 - Implement WebProcess freezer opt-in completely on WebContent process side
Summary: Implement WebProcess freezer opt-in completely on WebContent process side
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:
Blocks: 196062
  Show dependency treegraph
 
Reported: 2019-03-22 08:39 PDT by Chris Dumez
Modified: 2019-03-25 08:41 PDT (History)
6 users (show)

See Also:


Attachments
WIP Patch (8.81 KB, patch)
2019-03-22 09:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (9.27 KB, patch)
2019-03-22 09:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (9.27 KB, patch)
2019-03-22 09:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (9.27 KB, patch)
2019-03-22 09:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (14.98 KB, patch)
2019-03-22 09:56 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 Chris Dumez 2019-03-22 08:39:53 PDT
Implement WebProcess freezer opt-in completely on WebContent process side, we do not need to involve the UIProcess with this and rely on IPC.
Comment 1 Chris Dumez 2019-03-22 09:03:35 PDT
Created attachment 365730 [details]
WIP Patch
Comment 2 Brady Eidson 2019-03-22 09:08:36 PDT
(In reply to Chris Dumez from comment #0)
> Implement WebProcess freezer opt-in completely on WebContent process side,
> we do not need to involve the UIProcess with this and rely on IPC.

I think we actually want the UIProcess to be the *only one* in charge of this, and I've actually asked to make that possible.

I think that's the best way to get rid of this silly IPC.
Comment 3 Chris Dumez 2019-03-22 09:19:55 PDT
(In reply to Brady Eidson from comment #2)
> (In reply to Chris Dumez from comment #0)
> > Implement WebProcess freezer opt-in completely on WebContent process side,
> > we do not need to involve the UIProcess with this and rely on IPC.
> 
> I think we actually want the UIProcess to be the *only one* in charge of
> this, and I've actually asked to make that possible.
> 
> I think that's the best way to get rid of this silly IPC.

If and when we get the API to do this from the UIProcess, we can always move the 2 lines of code in the UIProcess. In the mean time, I still think than doing this purely on the WebProcess is more reliable and way less code.
Comment 4 Chris Dumez 2019-03-22 09:21:59 PDT
Created attachment 365732 [details]
WIP Patch
Comment 5 Brady Eidson 2019-03-22 09:29:31 PDT
Comment on attachment 365730 [details]
WIP Patch

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

I think this lets prewarmed processes be freezable, which we definitely don't want.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:696
> +    auto result = memorystatus_set_process_is_freezable(getpid(), shouldFreezeOnSuspension() ? 1 : 0);

Shouldn't use this directly. Please use the form of the code that you removed.
Comment 6 Chris Dumez 2019-03-22 09:29:57 PDT
(In reply to Brady Eidson from comment #5)
> Comment on attachment 365730 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365730&action=review
> 
> I think this lets prewarmed processes be freezable, which we definitely
> don't want.
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:696
> > +    auto result = memorystatus_set_process_is_freezable(getpid(), shouldFreezeOnSuspension() ? 1 : 0);
> 
> Shouldn't use this directly. Please use the form of the code that you
> removed.

Yes, that's already fixed.
Comment 7 Chris Dumez 2019-03-22 09:41:20 PDT
(In reply to Brady Eidson from comment #5)
> Comment on attachment 365730 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365730&action=review
> 
> I think this lets prewarmed processes be freezable, which we definitely
> don't want.

I will explicitly opt these out but Antti confirmed that prewarmed WebProcess do not have a WebPage and therefore would not be freezable with my current heuristic?
Comment 8 Chris Dumez 2019-03-22 09:41:42 PDT
Created attachment 365735 [details]
WIP Patch
Comment 9 Chris Dumez 2019-03-22 09:48:27 PDT
Created attachment 365736 [details]
WIP Patch
Comment 10 Chris Dumez 2019-03-22 09:56:39 PDT
Created attachment 365738 [details]
Patch
Comment 11 Chris Dumez 2019-03-22 09:56:56 PDT
Tested it on device.
Comment 12 Brady Eidson 2019-03-22 10:16:40 PDT
Comment on attachment 365738 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:693
> +    switch (m_processType) {
> +    case ProcessType::Inspector:
> +    case ProcessType::ServiceWorker:
> +    case ProcessType::PrewarmedWebContent:
> +    case ProcessType::CachedWebContent:
> +        return false;
> +    case ProcessType::WebContent:
> +        break;
> +    }

I like how explicitly this is about prewarmed/cached. 👍
Comment 13 Chris Dumez 2019-03-22 10:22:20 PDT
(In reply to Brady Eidson from comment #12)
> Comment on attachment 365738 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365738&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:693
> > +    switch (m_processType) {
> > +    case ProcessType::Inspector:
> > +    case ProcessType::ServiceWorker:
> > +    case ProcessType::PrewarmedWebContent:
> > +    case ProcessType::CachedWebContent:
> > +        return false;
> > +    case ProcessType::WebContent:
> > +        break;
> > +    }
> 
> I like how explicitly this is about prewarmed/cached. 👍

Agreed, this is more robust and clearer when reading the code.
Comment 14 WebKit Commit Bot 2019-03-22 10:40:57 PDT
Comment on attachment 365738 [details]
Patch

Clearing flags on attachment: 365738

Committed r243388: <https://trac.webkit.org/changeset/243388>
Comment 15 WebKit Commit Bot 2019-03-22 10:40:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-03-22 10:44:18 PDT
<rdar://problem/49159474>
Comment 17 Chris Dumez 2019-03-22 11:49:52 PDT
Follow-up build fix: <https://trac.webkit.org/changeset/243395>
Comment 18 Ryan Haddad 2019-03-22 14:40:12 PDT
(In reply to WebKit Commit Bot from comment #14)
> Comment on attachment 365738 [details]
> Patch
> 
> Clearing flags on attachment: 365738
> 
> Committed r243388: <https://trac.webkit.org/changeset/243388>
This change caused ~60 ProcessSwap API tests to fail an assertion on iOS Simulator:
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/builds/2943/steps/run-api-tests/logs/stdio

ASSERTION FAILED: m_processType == ProcessType::PrewarmedWebContent
        /Volumes/Data/slave/ios-simulator-12-debug/build/Source/WebKit/WebProcess/WebProcess.cpp(524) : void WebKit::WebProcess::markIsNoLongerPrewarmed()
        1   0x11bcdcf39 WTFCrash
        2   0x11009a06b WTFCrashWithInfo(int, char const*, char const*, int)
        3   0x110e3aacb WebKit::WebProcess::markIsNoLongerPrewarmed()
Comment 19 Chris Dumez 2019-03-25 08:23:32 PDT
(In reply to Ryan Haddad from comment #18)
> (In reply to WebKit Commit Bot from comment #14)
> > Comment on attachment 365738 [details]
> > Patch
> > 
> > Clearing flags on attachment: 365738
> > 
> > Committed r243388: <https://trac.webkit.org/changeset/243388>
> This change caused ~60 ProcessSwap API tests to fail an assertion on iOS
> Simulator:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/builds/2943/steps/run-
> api-tests/logs/stdio
> 
> ASSERTION FAILED: m_processType == ProcessType::PrewarmedWebContent
>        
> /Volumes/Data/slave/ios-simulator-12-debug/build/Source/WebKit/WebProcess/
> WebProcess.cpp(524) : void WebKit::WebProcess::markIsNoLongerPrewarmed()
>         1   0x11bcdcf39 WTFCrash
>         2   0x11009a06b WTFCrashWithInfo(int, char const*, char const*, int)
>         3   0x110e3aacb WebKit::WebProcess::markIsNoLongerPrewarmed()

Looking now. Probably a minor issue.
Comment 20 Chris Dumez 2019-03-25 08:41:40 PDT
(In reply to Chris Dumez from comment #19)
> (In reply to Ryan Haddad from comment #18)
> > (In reply to WebKit Commit Bot from comment #14)
> > > Comment on attachment 365738 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 365738
> > > 
> > > Committed r243388: <https://trac.webkit.org/changeset/243388>
> > This change caused ~60 ProcessSwap API tests to fail an assertion on iOS
> > Simulator:
> > https://build.webkit.org/builders/
> > Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/builds/2943/steps/run-
> > api-tests/logs/stdio
> > 
> > ASSERTION FAILED: m_processType == ProcessType::PrewarmedWebContent
> >        
> > /Volumes/Data/slave/ios-simulator-12-debug/build/Source/WebKit/WebProcess/
> > WebProcess.cpp(524) : void WebKit::WebProcess::markIsNoLongerPrewarmed()
> >         1   0x11bcdcf39 WTFCrash
> >         2   0x11009a06b WTFCrashWithInfo(int, char const*, char const*, int)
> >         3   0x110e3aacb WebKit::WebProcess::markIsNoLongerPrewarmed()
> 
> Looking now. Probably a minor issue.

Fix for API tests landed in <https://trac.webkit.org/changeset/243439>.