[Process-Swap-On-Navigation] WebKit hangs when going back to a form submission's page due to Process-Swap-On-Navigation on iOS 12.2 and higher

• Component: Page Loading
• Status: RESOLVED FIXED

Summary:
When going back to a page loaded when a form is submitted after having reloaded it fails.

Steps to Reproduce:
On Safari:
1. Go to a form page (e.g. https://rsolomakhin.github.io/autofill/)
2. Submit the form
3. Reload the page
4. Go back
5. Go forward

Expected Results:
The page loaded when the form is submitted should be loaded.

Actual Results:
The load fails with [ProcessSwapping] 0x14a426000 - ProvisionalPageProxy::didFailProvisionalLoadForFrame: pageID = 1, frameID = 2, navigationID = 18

I mentioned Safari but I reproduced with WebKit.

[Chris Dumez]

I am having trouble reproducing.

Which of the forms on this page are you submitting?
Also, at step 3, when I reload, it ask me if I want to resubmit. Do you say yes?

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> I am having trouble reproducing.
> 
> Which of the forms on this page are you submitting?
> Also, at step 3, when I reload, it ask me if I want to resubmit. Do you say
> yes?

Oh, never mind, I can reproduce. Will take a look

[Radar WebKit Bug Importer]

<rdar://problem/50048318>

[Radar WebKit Bug Importer]

<rdar://problem/50048319>

[Chris Dumez]

Seems to reproduce on macOS too.

[Chris Dumez]

Seems to fail at networking level?

0x11563aa00 - NetworkResourceLoader::didFailLoading: (pageID = 5, frameID = 4, resourceID = 6, isTimeout = 0, isCancellation = 0, isAccessControl = 0, errCode = -1008)

kCFURLErrorResourceUnavailable = -1008

[Chris Dumez]

I think this is likely due to the fact the the FormData is stored on the HistoryItem in the WebContent process, but not on the WebBackForwardListItem in the UIProcess. As a result, we lose the FormData on process-swap.

[Chris Dumez]

Ok, this is similar to the content filtering bug Andy and I looked at recently. We first try to do a load with MayAttemptCacheOnlyLoadForFormSubmissionItem, which fails. WebCore is then supposed to retry with MayNotAttemptCacheOnlyLoadForFormSubmissionItem.

However, because we send a didFailProvisionalLoad to the UIProcess, the ProvisionalPageProxy gets destroyed before the provisional process gets a change to retry.

[Chris Dumez]

I have a fix, working on a test.

[Chris Dumez]

Created attachment 367820 [details]
WIP Patch

[Chris Dumez]

Created attachment 367827 [details]
WIP Patch

[Chris Dumez]

Created attachment 367844 [details]
WIP Patch

[Chris Dumez]

Created attachment 367853 [details]
Patch

[EWS Watchlist]

Attachment 367853 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:87:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4286:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4293:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 367947 [details]
Patch

[EWS Watchlist]

Attachment 367947 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:87:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4286:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4293:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 367947 [details]
Patch

Looking into the iOS API test failure.

[Chris Dumez]

Created attachment 367968 [details]
Patch

[EWS Watchlist]

Attachment 367968 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:87:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4286:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4293:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 367968 [details]
Patch

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

> Source/WebCore/loader/FrameLoaderTypes.h:70
> +enum class WillContinueLoading : bool { No, Yes };

Does this mean it will continue loading in the same process or in another process?

> Source/WebCore/loader/FrameLoaderTypes.h:222
> +template <> struct EnumTraits<WebCore::WillContinueLoading> {

This isn't needed for enum classes that have a bool storage class.

> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:73
> +- (BOOL)_onlyIfCached

This is not a good name.

[Chris Dumez]

Comment on attachment 367968 [details]
Patch

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

>> Source/WebCore/loader/FrameLoaderTypes.h:70
>> +enum class WillContinueLoading : bool { No, Yes };
> 
> Does this mean it will continue loading in the same process or in another process?

We're in WebCore so this shouldn't have anything to do with processes. This is a flag that is associated with the DidFailProvisionalLoad IPC that is being sent by the WebProcess to the UIProcess. It means, even though I failed provisional, I'll keep loading.
If you really want to know, it will keep going in this process.

>> Source/WebCore/loader/FrameLoaderTypes.h:222
>> +template <> struct EnumTraits<WebCore::WillContinueLoading> {
> 
> This isn't needed for enum classes that have a bool storage class.

Ok, I seem to remember I added it due to a build error but I'll try and remove it.

>> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:73
>> +- (BOOL)_onlyIfCached
> 
> This is not a good name.

This is the official name in the fetch spec: "only-if-cached". We use FetchOptions::Cache::OnlyIfCached internally. I personally think it is pretty clear but do you have a better suggestion?

[Chris Dumez]

Created attachment 367982 [details]
Patch

[EWS Watchlist]

Attachment 367982 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:87:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4286:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4293:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 367982 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:73
> +- (BOOL)_onlyIfCached

I guess this makes sense when I think about it.  It seems like more of a property of the request than the task.  _requestOnlyIfCached?

[Chris Dumez]

(In reply to Alex Christensen from comment #25)
> Comment on attachment 367982 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367982&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:73
> > +- (BOOL)_onlyIfCached
> 
> I guess this makes sense when I think about it.  It seems like more of a
> property of the request than the task.  _requestOnlyIfCached?

Fair enough.

[Chris Dumez]

Committed r244540: <https://trac.webkit.org/changeset/244540>

[Shawn Roberts]

After changes in https://trac.webkit.org/changeset/244540

The following API test is crashing on iOS Sim Debug

TestWebKitAPI.ProcessSwap.GetUserMediaCaptureState

Test crashes in r244540, passes in r244539

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib               	0x000000010f78e64b objc_msgSend + 11
1   com.apple.CFNetwork           	0x00000001110ffa2f -[NSURLRequest dealloc] + 36
2   libobjc.A.dylib               	0x000000010f78c72c objc_object::sidetable_release(bool) + 202
3   libobjc.A.dylib               	0x000000010f78ce7f (anonymous namespace)::AutoreleasePoolPage::pop(void*) + 779
4   com.apple.CoreFoundation      	0x000000011028bdf6 _CFAutoreleasePoolPop + 22
5   com.apple.Foundation          	0x000000010f1a634d -[NSAutoreleasePool drain] + 144
6   TestWebKitAPI                 	0x00000001086fa987 main + 455 (mainIOS.mm:49)
7   libdyld.dylib                 	0x0000000110a48541 start + 1

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

[Chris Dumez]

(In reply to Shawn Roberts from comment #28)
> After changes in https://trac.webkit.org/changeset/244540
> 
> The following API test is crashing on iOS Sim Debug
> 
> TestWebKitAPI.ProcessSwap.GetUserMediaCaptureState
> 
> Test crashes in r244540, passes in r244539
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   libobjc.A.dylib               	0x000000010f78e64b objc_msgSend + 11
> 1   com.apple.CFNetwork           	0x00000001110ffa2f -[NSURLRequest
> dealloc] + 36
> 2   libobjc.A.dylib               	0x000000010f78c72c
> objc_object::sidetable_release(bool) + 202
> 3   libobjc.A.dylib               	0x000000010f78ce7f (anonymous
> namespace)::AutoreleasePoolPage::pop(void*) + 779
> 4   com.apple.CoreFoundation      	0x000000011028bdf6 _CFAutoreleasePoolPop
> + 22
> 5   com.apple.Foundation          	0x000000010f1a634d -[NSAutoreleasePool
> drain] + 144
> 6   TestWebKitAPI                 	0x00000001086fa987 main + 455
> (mainIOS.mm:49)
> 7   libdyld.dylib                 	0x0000000110a48541 start + 1
> 
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3433/steps/
> run-api-tests/logs/stdio

This was fixed in http://trac.webkit.org/r244613.