Bug 189481 – Regression(PSON): WebView is blank when navigating back cross-process on iOS

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

WebView is blank when navigating back cross-process on iOS (due to process swap on navigation being enabled).

[Radar WebKit Bug Importer]

<rdar://problem/44315010>

[Chris Dumez]

Created attachment 349325 [details]
Patch

[Tim Horton]

Comment on attachment 349325 [details]
Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:526
> +    if (auto* drawingArea = m_page.drawingArea())

I bet these aren't the only null checks we need.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6003
> +    if (m_isSuspended == suspended)
> +        return;

Suspended just means PSON-y suspension, not e.g. normal process suspension?

[Chris Dumez]

(In reply to Tim Horton from comment #3)
> Comment on attachment 349325 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349325&action=review
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:526
> > +    if (auto* drawingArea = m_page.drawingArea())
> 
> I bet these aren't the only null checks we need.

Chances are you are right. Those are the only ones I hit on on Mac and iOS so far though.

> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6003
> > +    if (m_isSuspended == suspended)
> > +        return;
> 
> Suspended just means PSON-y suspension, not e.g. normal process suspension?

PSON-y suspension.

[Tim Horton]

(In reply to Chris Dumez from comment #4)
> (In reply to Tim Horton from comment #3)
> > Comment on attachment 349325 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=349325&action=review
> > 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:526
> > > +    if (auto* drawingArea = m_page.drawingArea())
> > 
> > I bet these aren't the only null checks we need.
> 
> Chances are you are right. Those are the only ones I hit on on Mac and iOS
> so far though.
> 
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6003
> > > +    if (m_isSuspended == suspended)
> > > +        return;
> > 
> > Suspended just means PSON-y suspension, not e.g. normal process suspension?
> 
> PSON-y suspension.

Very mysterious, we should probably think about naming these things more independently.

[Chris Dumez]

(In reply to Tim Horton from comment #5)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Tim Horton from comment #3)
> > > Comment on attachment 349325 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=349325&action=review
> > > 
> > > > Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:526
> > > > +    if (auto* drawingArea = m_page.drawingArea())
> > > 
> > > I bet these aren't the only null checks we need.
> > 
> > Chances are you are right. Those are the only ones I hit on on Mac and iOS
> > so far though.
> > 
> > > 
> > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6003
> > > > +    if (m_isSuspended == suspended)
> > > > +        return;
> > > 
> > > Suspended just means PSON-y suspension, not e.g. normal process suspension?
> > 
> > PSON-y suspension.
> 
> Very mysterious, we should probably think about naming these things more
> independently.

I agree, Brady added this and I am not quite sure what he had in mind at the time. I'll clarify the naming in a follow-up.

[WebKit Commit Bot]

Comment on attachment 349325 [details]
Patch

Clearing flags on attachment: 349325

Committed r235867: <https://trac.webkit.org/changeset/235867>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dawei Fenton (:realdawei)]

(In reply to WebKit Commit Bot from comment #7)
> Comment on attachment 349325 [details]
> Patch
> 
> Clearing flags on attachment: 349325
> 
> Committed r235867: <https://trac.webkit.org/changeset/235867>

Seeing API failures on macOS and iOS after this revision

sample output: 
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/7888/steps/run-api-tests/logs/stdio

 TestWebKitAPI.ProcessSwap.ProcessReuse
        Received data during response processing, queuing it.
        WARNING: The option to always keep swapped web processes alive is active. This is meant for debugging and testing only.
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1375
        Expected equality of these values:
          2u
            Which is: 2
          seenPIDs.size()
            Which is: 3
        
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1378
        Expected equality of these values:
          pid1
            Which is: 76466
          pid3
            Which is: 76470
        

    TestWebKitAPI.ProcessSwap.ProcessReuseeTLDPlus2
        Received data during response processing, queuing it.
        WARNING: The option to always keep swapped web processes alive is active. This is meant for debugging and testing only.
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1422
        Expected equality of these values:
          2u
            Which is: 2
          seenPIDs.size()
            Which is: 3
        
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1425
        Expected equality of these values:
          pid1
            Which is: 76472
          pid3

[Chris Dumez]

(In reply to Dawei Fenton (:realdawei) from comment #9)
> (In reply to WebKit Commit Bot from comment #7)
> > Comment on attachment 349325 [details]
> > Patch
> > 
> > Clearing flags on attachment: 349325
> > 
> > Committed r235867: <https://trac.webkit.org/changeset/235867>
> 
> Seeing API failures on macOS and iOS after this revision
> 
> sample output: 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/7888/steps/run-api-
> tests/logs/stdio
> 
>  TestWebKitAPI.ProcessSwap.ProcessReuse
>         Received data during response processing, queuing it.
>         WARNING: The option to always keep swapped web processes alive is
> active. This is meant for debugging and testing only.
>         Received data during response processing, queuing it.
>         Received data during response processing, queuing it.
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessSwapOnNavigation.mm:1375
>         Expected equality of these values:
>           2u
>             Which is: 2
>           seenPIDs.size()
>             Which is: 3
>         
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessSwapOnNavigation.mm:1378
>         Expected equality of these values:
>           pid1
>             Which is: 76466
>           pid3
>             Which is: 76470
>         
> 
>     TestWebKitAPI.ProcessSwap.ProcessReuseeTLDPlus2
>         Received data during response processing, queuing it.
>         WARNING: The option to always keep swapped web processes alive is
> active. This is meant for debugging and testing only.
>         Received data during response processing, queuing it.
>         Received data during response processing, queuing it.
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessSwapOnNavigation.mm:1422
>         Expected equality of these values:
>           2u
>             Which is: 2
>           seenPIDs.size()
>             Which is: 3
>         
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessSwapOnNavigation.mm:1425
>         Expected equality of these values:
>           pid1
>             Which is: 76472
>           pid3

Odd, I will investigate shortly.

[Chris Dumez]

(In reply to Dawei Fenton (:realdawei) from comment #9)
> (In reply to WebKit Commit Bot from comment #7)
> > Comment on attachment 349325 [details]
> > Patch
> > 
> > Clearing flags on attachment: 349325
> > 
> > Committed r235867: <https://trac.webkit.org/changeset/235867>
> 
> Seeing API failures on macOS and iOS after this revision
> 
> sample output: 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/7888/steps/run-api-
> tests/logs/stdio
> 
>  TestWebKitAPI.ProcessSwap.ProcessReuse
>         Received data during response processing, queuing it.
>         WARNING: The option to always keep swapped web processes alive is
> active. This is meant for debugging and testing only.
>         Received data during response processing, queuing it.
>         Received data during response processing, queuing it.
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessSwapOnNavigation.mm:1375
>         Expected equality of these values:
>           2u
>             Which is: 2
>           seenPIDs.size()
>             Which is: 3
>         
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessSwapOnNavigation.mm:1378
>         Expected equality of these values:
>           pid1
>             Which is: 76466
>           pid3
>             Which is: 76470
>         
> 
>     TestWebKitAPI.ProcessSwap.ProcessReuseeTLDPlus2
>         Received data during response processing, queuing it.
>         WARNING: The option to always keep swapped web processes alive is
> active. This is meant for debugging and testing only.
>         Received data during response processing, queuing it.
>         Received data during response processing, queuing it.
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessSwapOnNavigation.mm:1422
>         Expected equality of these values:
>           2u
>             Which is: 2
>           seenPIDs.size()
>             Which is: 3
>         
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessSwapOnNavigation.mm:1425
>         Expected equality of these values:
>           pid1
>             Which is: 76472
>           pid3

Follow-up fix in <https://trac.webkit.org/changeset/235880>.

[Chris Dumez]

(In reply to Chris Dumez from comment #11)
> (In reply to Dawei Fenton (:realdawei) from comment #9)
> > (In reply to WebKit Commit Bot from comment #7)
> > > Comment on attachment 349325 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 349325
> > > 
> > > Committed r235867: <https://trac.webkit.org/changeset/235867>
> > 
> > Seeing API failures on macOS and iOS after this revision
> > 
> > sample output: 
> > https://build.webkit.org/builders/
> > Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/7888/steps/run-api-
> > tests/logs/stdio
> > 
> >  TestWebKitAPI.ProcessSwap.ProcessReuse
> >         Received data during response processing, queuing it.
> >         WARNING: The option to always keep swapped web processes alive is
> > active. This is meant for debugging and testing only.
> >         Received data during response processing, queuing it.
> >         Received data during response processing, queuing it.
> >         
> >        
> > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/ProcessSwapOnNavigation.mm:1375
> >         Expected equality of these values:
> >           2u
> >             Which is: 2
> >           seenPIDs.size()
> >             Which is: 3
> >         
> >         
> >        
> > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/ProcessSwapOnNavigation.mm:1378
> >         Expected equality of these values:
> >           pid1
> >             Which is: 76466
> >           pid3
> >             Which is: 76470
> >         
> > 
> >     TestWebKitAPI.ProcessSwap.ProcessReuseeTLDPlus2
> >         Received data during response processing, queuing it.
> >         WARNING: The option to always keep swapped web processes alive is
> > active. This is meant for debugging and testing only.
> >         Received data during response processing, queuing it.
> >         Received data during response processing, queuing it.
> >         
> >        
> > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/ProcessSwapOnNavigation.mm:1422
> >         Expected equality of these values:
> >           2u
> >             Which is: 2
> >           seenPIDs.size()
> >             Which is: 3
> >         
> >         
> >        
> > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/ProcessSwapOnNavigation.mm:1425
> >         Expected equality of these values:
> >           pid1
> >             Which is: 76472
> >           pid3
> 
> Follow-up fix in <https://trac.webkit.org/changeset/235880>.

Dawei, do you still see failures after the fix? I see that you just cc'd a few more people.

[Dawei Fenton (:realdawei)]

(In reply to Chris Dumez from comment #12)
> (In reply to Chris Dumez from comment #11)
> > (In reply to Dawei Fenton (:realdawei) from comment #9)
> > > (In reply to WebKit Commit Bot from comment #7)
> > > > Comment on attachment 349325 [details]
> > > > Patch
> > > > 
> > > > Clearing flags on attachment: 349325
> > > > 
> > > > Committed r235867: <https://trac.webkit.org/changeset/235867>
> > > 
> > > Seeing API failures on macOS and iOS after this revision
> > > 
> > > sample output: 
> > > https://build.webkit.org/builders/
> > > Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/7888/steps/run-api-
> > > tests/logs/stdio
> > > 
> > >  TestWebKitAPI.ProcessSwap.ProcessReuse
> > >         Received data during response processing, queuing it.
> > >         WARNING: The option to always keep swapped web processes alive is
> > > active. This is meant for debugging and testing only.
> > >         Received data during response processing, queuing it.
> > >         Received data during response processing, queuing it.
> > >         
> > >        
> > > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > > WebKitCocoa/ProcessSwapOnNavigation.mm:1375
> > >         Expected equality of these values:
> > >           2u
> > >             Which is: 2
> > >           seenPIDs.size()
> > >             Which is: 3
> > >         
> > >         
> > >        
> > > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > > WebKitCocoa/ProcessSwapOnNavigation.mm:1378
> > >         Expected equality of these values:
> > >           pid1
> > >             Which is: 76466
> > >           pid3
> > >             Which is: 76470
> > >         
> > > 
> > >     TestWebKitAPI.ProcessSwap.ProcessReuseeTLDPlus2
> > >         Received data during response processing, queuing it.
> > >         WARNING: The option to always keep swapped web processes alive is
> > > active. This is meant for debugging and testing only.
> > >         Received data during response processing, queuing it.
> > >         Received data during response processing, queuing it.
> > >         
> > >        
> > > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > > WebKitCocoa/ProcessSwapOnNavigation.mm:1422
> > >         Expected equality of these values:
> > >           2u
> > >             Which is: 2
> > >           seenPIDs.size()
> > >             Which is: 3
> > >         
> > >         
> > >        
> > > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > > WebKitCocoa/ProcessSwapOnNavigation.mm:1425
> > >         Expected equality of these values:
> > >           pid1
> > >             Which is: 76472
> > >           pid3
> > 
> > Follow-up fix in <https://trac.webkit.org/changeset/235880>.
> 
> Dawei, do you still see failures after the fix? I see that you just cc'd a
> few more people.

Sorry about that, we generally cc each other to facilitate tracking...I had forgotten to cc earlier :)