Bug 228683 – WebKitBlobResource error 1 exactly after 60 seconds when trying to read file input

• Component: Platform
• Status: RESOLVED FIXED

[Jan Lenczyk]

When trying to parse large files in chunks using FileReader for example, mobile safari seems to somehow revoke access to a file input after exactly 60 seconds. Expected behaviour would be that the file input stays available until changed by the user. This does not happen in desktop safari. Only on mobile.

JSFiddle to reproduce this behaviour on mobile safari (inputs file, waits 70 seconds and then attempts to read it): https://jsfiddle.net/rw8m1k29/

[Alexey Proskuryakov]

For Apple employees, see also rdar://78448610.

[Jan Lenczyk]

Edit: Also happens in the UIWebview & WKWebview.

[Alex Christensen]

Perhaps I'm misunderstanding something.  I see this after 70 seconds:

fetching blob URI success
reader success
blob URI success

I see this on my Mac and my iPhone.

[Alex Christensen]

Aha!  I saw that success when uploading a photo.  If I instead select "Choose File" I hit the failure.  Now I can investigate this bug.

[Jan Lenczyk]

Thanks for your reply. I was pretty confused at first since I've tested it on literally every device I could get my hands on and saw the same result everytime. Good that you are able to investigate this now. If you need any more input I'll help as much as possible. I've currently tested this on Mobile Safari, WKWebiew and Capacitor/Cordova (both also use WKWebview) - all on the latest possible version.

[Jan Lenczyk]

On Desktop Safari a file input stays available forever or until changed by the user, that's why I was a bit confused when I discovered this.

[Alex Christensen]

I have found the underlying cause, and I must say I'm disappointed.  I'm working to find a good fix.

[Radar WebKit Bug Importer]

<rdar://problem/81657131>

[Alex Christensen]

Created attachment 435209 [details]
Patch

[Chris Dumez]

Comment on attachment 435209 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        I manually verified this makes the files able to upload after 60 seconds.

The issue with the proposed solution seems to be that the files on disk will not longer get removed promptly. As a matter of fact, they won't be removed until the machine is rebooted, which may be a long time from now. Am I missing something? If not, why is this OK?

[Chris Dumez]

Comment on attachment 435209 [details]
Patch

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

>> Source/WebKit/ChangeLog:10
>> +        I manually verified this makes the files able to upload after 60 seconds.
> 
> The issue with the proposed solution seems to be that the files on disk will not longer get removed promptly. As a matter of fact, they won't be removed until the machine is rebooted, which may be a long time from now. Am I missing something? If not, why is this OK?

Also, I am not familiar with this UIKit API (so I cc'd people who are) but are we certain it is OK for us to *move* (e.g. instead of copy) the files being passed to us? Are these *always* temporary files?

[Alex Christensen]

In my observation the files are temporary.

One compromise we could do is to keep the files around for a longer time, such as 10 minutes.  That would only work if the app is around for that amount of time, but that is already the case with the 1 minute timer.

Moving the files is fine.  They check if they're still there then delete if they are.

[Tim Horton]

Comment on attachment 435209 [details]
Patch

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

>>> Source/WebKit/ChangeLog:10
>>> +        I manually verified this makes the files able to upload after 60 seconds.
>> 
>> The issue with the proposed solution seems to be that the files on disk will not longer get removed promptly. As a matter of fact, they won't be removed until the machine is rebooted, which may be a long time from now. Am I missing something? If not, why is this OK?
> 
> Also, I am not familiar with this UIKit API (so I cc'd people who are) but are we certain it is OK for us to *move* (e.g. instead of copy) the files being passed to us? Are these *always* temporary files?

I believe UIDocumentPickerModeImport does mean they're always copies. But never cleaning them up seems like a regression.

[Geoffrey Garen]

> > Also, I am not familiar with this UIKit API (so I cc'd people who are) but are we certain it is OK for us to *move* (e.g. instead of copy) the files being passed to us? Are these *always* temporary files?
> 
> I believe UIDocumentPickerModeImport does mean they're always copies.

Yes: https://developer.apple.com/documentation/uikit/uidocumentpickerdelegate/2902364-documentpicker?language=objc

[Chris Dumez]

(In reply to Alex Christensen from comment #12)
> In my observation the files are temporary.
> 
> One compromise we could do is to keep the files around for a longer time,
> such as 10 minutes.  That would only work if the app is around for that
> amount of time, but that is already the case with the 1 minute timer.

Wouldn't it be possible/better to delete those files the next time a load gets committed in the view (and upon view destruction)? Seems like as long as the page is loaded, if you delete the files then there is a chance of this bug occurring (other less likely if you use 10min vs 1min).

> 
> Moving the files is fine.  They check if they're still there then delete if
> they are.

[Geoffrey Garen]

Maybe WKFileUploadPanel's destructor should remove the files?

Alternatively, maybe FileInputType::setFiles() or something along that path should open the files right away, to better match the platform programming expectation? (If we did that, we would not need to move the file to a new temporary location.)

[Alex Christensen]

Created attachment 435214 [details]
Patch

[Geoffrey Garen]

Comment on attachment 435214 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:677
> +        // UIKit would delete these temporary files after 1 minute. We clean up after 10 minutes.
> +        auto scheduleCleanup = [] (NSArray<NSURL *> *urls) {

I hate to have the patch hen-pecked to death, but I'm not really happy with this approach. Increasing the delay from 1 minute to 10 will solve forms that take between 1 minute and 10 minutes before the user submits, but will leave forms in the > 10 minute envelope broken. For example, if a user starts filling out a form, takes a break, and then finishes and submits the form -- that case will still be broken.

The requirements we need to meet are:

1. User can take any time they need before hitting submit;

2. If a temporary file is created, it must be reliably removed when we're done with it (by WebKit or by some platform mechanism, possibly after some reasonable delay if needed).

[Geoffrey Garen]

Related: The UI process may exit / background jetsam in < 10 minutes.

[Alex Christensen]

Created attachment 435228 [details]
Patch

[Alex Christensen]

WKFileUploadPanel's lifetime isn't nearly long enough for this, but WKContentView's is.

[Tim Horton]

Comment on attachment 435228 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1209
> +    if (_temporaryURLsToDeleteWhenDeallocated) {

I think you need to move this to dealloc (this code runs when the view is unparented as well, like in the tab switch case)

[Tim Horton]

(In reply to Tim Horton from comment #22)
> Comment on attachment 435228 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435228&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1209
> > +    if (_temporaryURLsToDeleteWhenDeallocated) {
> 
> I think you need to move this to dealloc (this code runs when the view is
> unparented as well, like in the tab switch case)

Nevermind, we don't seem to do that anymore (only cleanup a subset). I think this is fine.

[Tim Horton]

Comment on attachment 435228 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:522
> +    RetainPtr<NSMutableArray<NSURL *>> _temporaryURLsToDeleteWhenDeallocated;

Vaguely unclear why this isn't a Vector<RetainPtr<NSURL>> but this is fine too

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1213
> +            auto manager = adoptNS([[NSFileManager alloc] init]);

I think you can just use the defaultManager (and no adoption of course)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1219
> +                [coordinator coordinateWritingItemAtURL:url options:NSFileCoordinatorWritingForDeleting error:&error byAccessor:^(NSURL *coordinatedURL) {

When I suggested file coordination APIs I meant "you may be able to use them prevent UIKit from deleting the file by indicating that you need it", but this is fine too (and I don't know what I was suggesting is actually possible).

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:673
> +                        // If moving fails, keep the original URL and our 60 second time limit before it is deleted. We tried our best to extend it.

"copying" not moving

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:674
> +                        [newURLs addObject:coordinatedOriginalURL];

In the case where you don't copy, you still add to newURLs and pass to WKContentView, which then deletes the containing directory, which you don't own. Isn't that wrong?

[Tim Horton]

Comment on attachment 435228 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1224
> +                    if (![manager removeItemAtURL:temporaryDirectory error:&error] || error)

`_removeTemporaryFilesAndDirectoriesWhenDeallocated` seems like something mighty useful for other people and like they might come along and adopt it, but the fact that it deletes the containing directory is ... not entirely obvious from the API. Maybe it should only delete URLs explicitly passed to it and it should be the caller's decision to add the directory URL? (might help you fix the other issue I mentioned too)

[Alex Christensen]

Comment on attachment 435228 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1213
>> +            auto manager = adoptNS([[NSFileManager alloc] init]);
> 
> I think you can just use the defaultManager (and no adoption of course)

No, that could have a delegate attached to it which could change its behavior.  We want just plain file operations.

[Alex Christensen]

Created attachment 435233 [details]
Patch

[Tim Horton]

(In reply to Alex Christensen from comment #26)
> Comment on attachment 435228 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435228&action=review
> 
> >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1213
> >> +            auto manager = adoptNS([[NSFileManager alloc] init]);
> > 
> > I think you can just use the defaultManager (and no adoption of course)
> 
> No, that could have a delegate attached to it which could change its
> behavior.  We want just plain file operations.

Technically you're supposed to make your own /if you want to use the delegate/ (docs agree), but I guess it's possible someone could put one on the default one (I see nothing preventing this, anyway).

[Tim Horton]

Comment on attachment 435233 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1225
> +                    if (![manager removeItemAtURL:temporaryDirectory error:&error] || error)
> +                        LOG_ERROR(OS_LOG_DEFAULT, "WKContentViewInteraction failed to remove empty directory at path %@ with error %@", temporaryDirectory, error);

This is still here??

[David Kilzer (:ddkilzer)]

<rdar://problem/78448610>

[Alex Christensen]

Comment on attachment 435233 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1225
>> +                        LOG_ERROR(OS_LOG_DEFAULT, "WKContentViewInteraction failed to remove empty directory at path %@ with error %@", temporaryDirectory, error);
> 
> This is still here??

oops

[Alex Christensen]

Created attachment 435258 [details]
Patch

[Tim Horton]

Comment on attachment 435258 [details]
Patch

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

You'll need 3 reviewers, but I can be one.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1214
> +    if (!_temporaryURLsToDeleteWhenDeallocated.isEmpty()) {

Also kind of weird this is on WKContentView*Interaction* category and not in WKContentView proper

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:667
> +                NSString *temporaryDirectory = FileSystem::createTemporaryDirectory([adoptNS([[NSUUID alloc] init]) UUIDString]);

Just NSUUID.UUID.UUIDString because that's a thing of beauty

[Alex Christensen]

(In reply to Tim Horton from comment #33)
> > Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:667
> > +                NSString *temporaryDirectory = FileSystem::createTemporaryDirectory([adoptNS([[NSUUID alloc] init]) UUIDString]);
> 
> Just NSUUID.UUID.UUIDString because that's a thing of beauty

But that adds to the autorelease pool unnecessarily.

[Alex Christensen]

Created attachment 435279 [details]
Patch

[Chris Dumez]

Comment on attachment 435279 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentView.mm:317
> +    [self _removeTemporaryFilesIfNecessary];

Can't we call this in _didCommitLoadForMainFrame too? Once we've navigated away from the page, is there any good reason to keep the temporary files on disk around?

> Source/WebKit/UIProcess/ios/WKContentView.mm:350
> +    _temporaryURLsToDeleteWhenDeallocated.appendVector(WTFMove(urls));

Do we need to keep track of files individually? Can't we just delete the temporary folder we created?

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:667
> +                NSString *temporaryDirectory = FileSystem::createTemporaryDirectory([adoptNS([[NSUUID alloc] init]) UUIDString]);

Why the UUID? createTemporaryDirectory() already takes care of generating a unique name, all we need to provide is a prefix AFAIK (e.g. "filePickerFiles-")

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:677
> +                    if (![manager copyItemAtURL:coordinatedOriginalURL toURL:coordinatedDestinationURL error:&error] || error) {

Why are we copying the files instead of moving them now? I thought moving was safe (and more efficient).

[Alex Christensen]

(In reply to Chris Dumez from comment #36)
> Comment on attachment 435279 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435279&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentView.mm:317
> > +    [self _removeTemporaryFilesIfNecessary];
> 
> Can't we call this in _didCommitLoadForMainFrame too? Once we've navigated
> away from the page, is there any good reason to keep the temporary files on
> disk around?
That would introduce race conditions between committing a navigation and form submission, which can happen after committing a navigation.  I chose not to to be conservative and avoid introducing edge case bugs.
> 
> > Source/WebKit/UIProcess/ios/WKContentView.mm:350
> > +    _temporaryURLsToDeleteWhenDeallocated.appendVector(WTFMove(urls));
> 
> Do we need to keep track of files individually? Can't we just delete the
> temporary folder we created?
This deletes the contents of the directory before deleting the empty directory, which I think is less prone to failure.
> 
> > Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:667
> > +                NSString *temporaryDirectory = FileSystem::createTemporaryDirectory([adoptNS([[NSUUID alloc] init]) UUIDString]);
> 
> Why the UUID? createTemporaryDirectory() already takes care of generating a
> unique name, all we need to provide is a prefix AFAIK (e.g.
> "filePickerFiles-")
We need the UUID because we may be given multiple files with the same name.  createTemporaryDirectory with a constant would overwrite them and cause problems in that case.
> 
> > Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:677
> > +                    if (![manager copyItemAtURL:coordinatedOriginalURL toURL:coordinatedDestinationURL error:&error] || error) {
> 
> Why are we copying the files instead of moving them now? I thought moving
> was safe (and more efficient).
With APFS and since we will only read the contents and UIKit will only delete the original, the two operations should be equivalent.  I copied instead of moved to be conservative in case we are given a file that is not a copy.

[Chris Dumez]

(In reply to Alex Christensen from comment #37)
> (In reply to Chris Dumez from comment #36)
> > Comment on attachment 435279 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=435279&action=review
> > 
> > > Source/WebKit/UIProcess/ios/WKContentView.mm:317
> > > +    [self _removeTemporaryFilesIfNecessary];
> > 
> > Can't we call this in _didCommitLoadForMainFrame too? Once we've navigated
> > away from the page, is there any good reason to keep the temporary files on
> > disk around?
> That would introduce race conditions between committing a navigation and
> form submission, which can happen after committing a navigation.  I chose
> not to to be conservative and avoid introducing edge case bugs.

I don't understand how form submission can happen after committing a navigation. The form submission triggers a navigation but the load won't get committed until we've received the response from the server. By design, if we've received a response from the server then it means we sent it the request (which contained the form data in case of form submission).

WebViews may stay alive for a long time so deleting the files only on view deallocation seems suboptimal.

> > 
> > > Source/WebKit/UIProcess/ios/WKContentView.mm:350
> > > +    _temporaryURLsToDeleteWhenDeallocated.appendVector(WTFMove(urls));
> > 
> > Do we need to keep track of files individually? Can't we just delete the
> > temporary folder we created?
> This deletes the contents of the directory before deleting the empty
> directory, which I think is less prone to failure.

What does prone to failure mean? Deleting a non-empty folder is trivial with Cocoa APIs and it is actually less error-prone to delete our whole temporary folder than having to keep track of which files we need to remove inside that folder.

Also, you say you delete the empty directory but I didn't see that logic in the patch (maybe I missed it).

> > 
> > > Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:667
> > > +                NSString *temporaryDirectory = FileSystem::createTemporaryDirectory([adoptNS([[NSUUID alloc] init]) UUIDString]);
> > 
> > Why the UUID? createTemporaryDirectory() already takes care of generating a
> > unique name, all we need to provide is a prefix AFAIK (e.g.
> > "filePickerFiles-")
> We need the UUID because we may be given multiple files with the same name. 
> createTemporaryDirectory with a constant would overwrite them and cause
> problems in that case.

My understanding is that createTemporaryDirectory already takes care of generating a unique name for you in the format "[PrefixYouProvide]-[XXXXXXXX]" where mkdtemp() will take care of choosing XXXXXXXX so that the name is unique. I don't understand what using a UUID for the prefix brings despite more computation.

> > 
> > > Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:677
> > > +                    if (![manager copyItemAtURL:coordinatedOriginalURL toURL:coordinatedDestinationURL error:&error] || error) {
> > 
> > Why are we copying the files instead of moving them now? I thought moving
> > was safe (and more efficient).
> With APFS and since we will only read the contents and UIKit will only
> delete the original, the two operations should be equivalent.  I copied
> instead of moved to be conservative in case we are given a file that is not
> a copy.

I didn't know this was as efficient with apfs. I guess that's fine then. I think moving would have been fine too given previous discussion.

[Alex Christensen]

(In reply to Chris Dumez from comment #38)
> I don't understand how form submission can happen after committing a
> navigation. The form submission triggers a navigation but the load won't get
> committed until we've received the response from the server. By design, if
> we've received a response from the server then it means we sent it the
> request (which contained the form data in case of form submission).

If something else besides the form submission causes a main frame navigation, then if between when the response is received and the next document replaces the current document a form is submitted using an iframe, the race condition I described could be hit.

> What does prone to failure mean? Deleting a non-empty folder is trivial with
> Cocoa APIs and it is actually less error-prone to delete our whole temporary
> folder than having to keep track of which files we need to remove inside
> that folder.
The API is indeed trivial, but there is an error parameter.  It can fail, and I coordinate deletion.  I also thought I had observed NSFileManager being unable to delete a non-empty directory, but now I have observed that it can.  I think I'll update this to just delete the whole directory.
 
> Also, you say you delete the empty directory but I didn't see that logic in
> the patch (maybe I missed it).
When creating the temp directory, I add the temp file to the vector then append the temp directory to the vector.  Will change, though.

> My understanding is that createTemporaryDirectory already takes care of
> generating a unique name for you in the format
> "[PrefixYouProvide]-[XXXXXXXX]" where mkdtemp() will take care of choosing
> XXXXXXXX so that the name is unique. I don't understand what using a UUID
> for the prefix brings despite more computation.
I thought I had observed this not happening, but now I think I was wrong.  If I'm wrong I will just use a fixed prefix instead of a UUID.

> I didn't know this was as efficient with apfs. I guess that's fine then. I
> think moving would have been fine too given previous discussion.
I'll move to leave fewer files if the cleanup fails.

[Chris Dumez]

(In reply to Alex Christensen from comment #39)
> (In reply to Chris Dumez from comment #38)
> > I don't understand how form submission can happen after committing a
> > navigation. The form submission triggers a navigation but the load won't get
> > committed until we've received the response from the server. By design, if
> > we've received a response from the server then it means we sent it the
> > request (which contained the form data in case of form submission).
> 
> If something else besides the form submission causes a main frame
> navigation, then if between when the response is received and the next
> document replaces the current document a form is submitted using an iframe,
> the race condition I described could be hit.

The scenario you're describing seems racy no matter what. Submitting a form is asynchronous and your top frame navigation may get committed before the form gets submitted (in the iframe) no matter what.

> 
> > What does prone to failure mean? Deleting a non-empty folder is trivial with
> > Cocoa APIs and it is actually less error-prone to delete our whole temporary
> > folder than having to keep track of which files we need to remove inside
> > that folder.
> The API is indeed trivial, but there is an error parameter.  It can fail,
> and I coordinate deletion.  I also thought I had observed NSFileManager
> being unable to delete a non-empty directory, but now I have observed that
> it can.  I think I'll update this to just delete the whole directory.
>  
> > Also, you say you delete the empty directory but I didn't see that logic in
> > the patch (maybe I missed it).
> When creating the temp directory, I add the temp file to the vector then
> append the temp directory to the vector.  Will change, though.
> 
> > My understanding is that createTemporaryDirectory already takes care of
> > generating a unique name for you in the format
> > "[PrefixYouProvide]-[XXXXXXXX]" where mkdtemp() will take care of choosing
> > XXXXXXXX so that the name is unique. I don't understand what using a UUID
> > for the prefix brings despite more computation.
> I thought I had observed this not happening, but now I think I was wrong. 
> If I'm wrong I will just use a fixed prefix instead of a UUID.
> 
> > I didn't know this was as efficient with apfs. I guess that's fine then. I
> > think moving would have been fine too given previous discussion.
> I'll move to leave fewer files if the cleanup fails.

[Alex Christensen]

Created attachment 435290 [details]
Patch

[Chris Dumez]

Comment on attachment 435290 [details]
Patch

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

LGTM.

> Source/WebKit/UIProcess/ios/WKContentView.mm:322
> +- (void)_removeTemporaryFilesIfNecessary

But I still think be better if we called called this from _didCommitLoadForMainFrame too, to avoid keeping unnecessary files on disk for too long.

Justifications for not doing so didn't make sense to me at least.

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:677
> +                    if (![manager moveItemAtURL:coordinatedOriginalURL toURL:destinationFileURL.get() error:&error] || error) {

I suspect we don't need the `|| error` check since moveItemAtURL would have returned NO in case of error.

[Alex Christensen]

Comment on attachment 435290 [details]
Patch

I'm being intentionally quite risk averse with this patch, given our timeline and the nature of this bug.  I wouldn't be opposed  to adding a call from _didCommitLoadForMainFrame after our next branch.

[EWS]

Committed r280875 (240413@main): <https://commits.webkit.org/240413@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435290 [details].

[Alex Christensen]

r280915

[Alex Christensen]

r280925

[Tim Horton]

Also https://trac.webkit.org/changeset/280883/webkit