Implement WebProcess freezer opt-in completely on WebContent process side

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Implement WebProcess freezer opt-in completely on WebContent process side, we do not need to involve the UIProcess with this and rely on IPC.

[Chris Dumez]

Created attachment 365730 [details]
WIP Patch

[Brady Eidson]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 365732 [details]
WIP Patch

[Brady Eidson]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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?

[Chris Dumez]

Created attachment 365735 [details]
WIP Patch

[Chris Dumez]

Created attachment 365736 [details]
WIP Patch

[Chris Dumez]

Created attachment 365738 [details]
Patch

[Chris Dumez]

Tested it on device.

[Brady Eidson]

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. 👍

[Chris Dumez]

(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.

[WebKit Commit Bot]

Comment on attachment 365738 [details]
Patch

Clearing flags on attachment: 365738

Committed r243388: <https://trac.webkit.org/changeset/243388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49159474>

[Chris Dumez]

Follow-up build fix: <https://trac.webkit.org/changeset/243395>

[Ryan Haddad]

(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()

[Chris Dumez]

(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.

[Chris Dumez]

(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>.