Navigation from empty page doesn't use cached web process

• Component: Page Loading
• Status: REOPENED ---

[Ben Nham]

When navigating from an empty page to another domain foo.com, we always use the source WebProcess (which is basically uninitialized) rather than using an already-initialized cached WebProcess that has navigated to foo.com. The cached WebProcess should probably be preferred since it has more relevant cached resources available to it (e.g. memory cache, JS bytecode cache, prewarmed fonts, ...).

[Ben Nham]

<rdar://problem/57703742>

[Ben Nham]

Created attachment 385162 [details]
Patch

[Ben Nham]

Comment on attachment 385162 [details]
Patch

For perf testing purposes, this mainly affects the first warm page load on Mac PLT5. The perf bots show that this speeds up that first page load by 10-15% across all devices; the overall impact is 0.4%-1.5% depending on the device.

[Chris Dumez]

Comment on attachment 385162 [details]
Patch

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

The patch is not safe as is because it causes a process-swap in cases where we know it is not OK to process-swap (e.g. when navigation.openedByDOMWithOpener() returns true). If a page does a window.open() of another, we cannot process swap due to the opener link.

> Source/WebKit/UIProcess/WebProcessPool.cpp:2142
> +        if (!targetRegistrableDomain.isEmpty()) {

We should write an API test for this. See existing ProcessSwap API tests.

[Alexey Proskuryakov]

> The patch is not safe as is because it causes a process-swap in cases where
> we know it is not OK to process-swap (e.g. when
> navigation.openedByDOMWithOpener() returns true). If a page does a
> window.open() of another, we cannot process swap due to the opener link.

Is it possible to add a test for this too?

[Ben Nham]

Created attachment 385697 [details]
test case for window.open (unused)

[Ben Nham]

Created attachment 385698 [details]
Patch

[Ben Nham]

Comment on attachment 385698 [details]
Patch

I discussed this offline with Chris and he thinks it's more correct and less error-prone in the future if we move the hasCommittedAnyProvisionalLoads block to near the end of the function after all the special cases (e.g. opened by window.open) have been handled. I've done that in this patch.

Note that there are already multiple test cases that test various window.open scenarios, e.g. "if a page does a window.open() of another, we cannot process swap due to the opener link". Those tests passed with the previous patch and still pass with the current patch with the hasCommittedAnyProvisionalLoads block moved down.

I spent quite a bit of time trying to write a rather convoluted test case that would cause a WebProcess to spawn with an empty document, while also being opened with window.open, but that led to nothing since the sourceProcess always ends up being the process that executed the window.open. See the attached unused test case for an example.

However, I did write a test case for the behavior that I added, which is that we should prefer a cached process over the current process if the current process hasn't committed any provisional loads.

[Chris Dumez]

Comment on attachment 385698 [details]
Patch

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

Looks good.

> Source/WebKit/ChangeLog:15
> +        Add an API test.

"Added"

> Source/WebKit/UIProcess/WebProcessPool.cpp:2208
> +            LOG(ProcessSwapping, "(ProcessSwapping) Reusing a previously cached process with pid %i to continue navigation to URL %s in place of pid %i that hasn't committed any provisional loads", process->processIdentifier(), targetURL.string().utf8().data(), sourceProcess->processIdentifier());

I don't mind you adding a LOG() that contains the URL since this is debug only. However, there should definitely be a RELEASE_LOG() like in the rest of this function to facilitate debugging in release builds. You also cannot log the URL or domain in the RELEASE_LOG() though.

[Ben Nham]

Created attachment 385776 [details]
Patch

[Ben Nham]

Comment on attachment 385776 [details]
Patch

The target domain in the LOG doesn't seem that useful (unless there is a bug in WebProcessCache, which would be a much bigger problem somewhere else), so just remove that and make it a RELEASE_LOG.

[WebKit Commit Bot]

Comment on attachment 385776 [details]
Patch

Clearing flags on attachment: 385776

Committed r253646: <https://trac.webkit.org/changeset/253646>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 205433