Bug 197097 - [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
Summary: [Process-Swap-On-Navigation] WebKit hangs when going back to a form submissio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-19 00:49 PDT by gambard
Modified: 2019-04-25 08:54 PDT (History)
10 users (show)

See Also:


Attachments
WIP Patch (25.82 KB, patch)
2019-04-19 13:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (27.55 KB, patch)
2019-04-19 14:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (32.61 KB, patch)
2019-04-19 15:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (42.16 KB, patch)
2019-04-19 16:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.53 KB, patch)
2019-04-22 10:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.74 KB, patch)
2019-04-22 13:40 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.25 KB, patch)
2019-04-22 14:55 PDT, Chris Dumez
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description gambard 2019-04-19 00:49:49 PDT
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
Comment 1 gambard 2019-04-19 01:47:02 PDT
I mentioned Safari but I reproduced with WebKit.
Comment 2 Chris Dumez 2019-04-19 08:25:48 PDT
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?
Comment 3 Chris Dumez 2019-04-19 08:26:47 PDT
(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
Comment 4 Radar WebKit Bug Importer 2019-04-19 08:27:16 PDT
<rdar://problem/50048318>
Comment 5 Radar WebKit Bug Importer 2019-04-19 08:27:17 PDT
<rdar://problem/50048319>
Comment 6 Chris Dumez 2019-04-19 08:28:26 PDT
Seems to reproduce on macOS too.
Comment 7 Chris Dumez 2019-04-19 08:35:53 PDT
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
Comment 8 Chris Dumez 2019-04-19 09:05:35 PDT
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.
Comment 9 Chris Dumez 2019-04-19 10:14:22 PDT
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.
Comment 10 Chris Dumez 2019-04-19 10:48:19 PDT
I have a fix, working on a test.
Comment 11 Chris Dumez 2019-04-19 13:57:02 PDT
Created attachment 367820 [details]
WIP Patch
Comment 12 Chris Dumez 2019-04-19 14:32:45 PDT
Created attachment 367827 [details]
WIP Patch
Comment 13 Chris Dumez 2019-04-19 15:27:05 PDT
Created attachment 367844 [details]
WIP Patch
Comment 14 Chris Dumez 2019-04-19 16:42:52 PDT
Created attachment 367853 [details]
Patch
Comment 15 Build Bot 2019-04-19 16:45:40 PDT
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.
Comment 16 Chris Dumez 2019-04-22 10:21:20 PDT
Created attachment 367947 [details]
Patch
Comment 17 Build Bot 2019-04-22 10:23:21 PDT
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.
Comment 18 Chris Dumez 2019-04-22 13:30:55 PDT
Comment on attachment 367947 [details]
Patch

Looking into the iOS API test failure.
Comment 19 Chris Dumez 2019-04-22 13:40:04 PDT
Created attachment 367968 [details]
Patch
Comment 20 Build Bot 2019-04-22 13:43:14 PDT
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.
Comment 21 Alex Christensen 2019-04-22 14:32:53 PDT
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.
Comment 22 Chris Dumez 2019-04-22 14:40:44 PDT
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?
Comment 23 Chris Dumez 2019-04-22 14:55:48 PDT
Created attachment 367982 [details]
Patch
Comment 24 Build Bot 2019-04-22 14:58:03 PDT
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.
Comment 25 Alex Christensen 2019-04-23 08:07:03 PDT
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?
Comment 26 Chris Dumez 2019-04-23 08:38:07 PDT
(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.
Comment 27 Chris Dumez 2019-04-23 08:38:18 PDT
Committed r244540: <https://trac.webkit.org/changeset/244540>
Comment 28 Shawn Roberts 2019-04-24 12:45:24 PDT
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
Comment 29 Chris Dumez 2019-04-25 08:54:36 PDT
(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.