Bug 192318 – Update process assertions after swapping processes

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Update process assertions after swapping processes

[Alex Christensen]

Created attachment 356395 [details]
Patch

[Alex Christensen]

<rdar://problem/46377552>

[Chris Dumez]

Comment on attachment 356395 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        after navigation and process swapping we need the new WebProcess to update its process assertions.

typo: and process swapping -> and *after* ?

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:1232
> +    // Take the process assertion from the old web process.

I would say:
// Transfer our background assertion from the old process to the new one.

[Alex Christensen]

Created attachment 356397 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

http://trac.webkit.org/r238817

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/238817/webkit

may have caused sever process swap api failures on iOS. 

build:
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/1316

output:
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/1316/steps/run-api-tests/logs/stdio

[Chris Dumez]

(In reply to Truitt Savell from comment #7)
> It looks like the changes in https://trac.webkit.org/changeset/238817/webkit
> 
> may have caused sever process swap api failures on iOS. 
> 
> build:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/1316
> 
> output:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/1316/steps/
> run-api-tests/logs/stdio

Seems to indicate Page Cache is not working. My bet is that the suspension relies on the process assertion on the old process, which gets dropped in this patch when we take the assertion for the new process.

If so, we may need to SuspendedPageProxy to hold a background token for its WebProcess until SuspendedPageProxy::didFinishLoad() is called.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to Truitt Savell from comment #7)
> > It looks like the changes in https://trac.webkit.org/changeset/238817/webkit
> > 
> > may have caused sever process swap api failures on iOS. 
> > 
> > build:
> > https://build.webkit.org/builders/
> > Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/1316
> > 
> > output:
> > https://build.webkit.org/builders/
> > Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/1316/steps/
> > run-api-tests/logs/stdio
> 
> Seems to indicate Page Cache is not working. My bet is that the suspension
> relies on the process assertion on the old process, which gets dropped in
> this patch when we take the assertion for the new process.
> 
> If so, we may need to SuspendedPageProxy to hold a background token for its
> WebProcess until SuspendedPageProxy::didFinishLoad() is called.

Dealing with those API test failures via Bug 192348.