Bug 138171 – FormData should not use Document, Page, Chrome and ChromeClient

• Status: NEW
• Component: Platform

[Carlos Garcia Campos]

It's a layering violation.

[Carlos Garcia Campos]

Created attachment 240599 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/xml/XMLHttpRequest.cpp:688:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/loader/FrameLoader.cpp:361:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 7 files


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

[Alexey Proskuryakov]

Comment on attachment 240599 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        FormData uses the ChromeClient to ask the WebKit layer whether to
> +        replace upload files with generated files and to actually
> +        generate the replacement files. We could use std::functions passed
> +        to FormData instead of calling ChromeClient methods directly.

This is OK to do, although a better fix would be to stop using the chrome client, and use file replacement code from BlobDataFileReference::generateReplacementFile(). That would require some refactoring, but the code is already in WebCore/platform.

Right now, we potentially archive a bundle twice, first via Blob code path if the file is ever accessed via File API, and then via the chrome client if the file end up being submitted via a form.

I did not review the patch in detail.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 240599 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240599&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        FormData uses the ChromeClient to ask the WebKit layer whether to
> > +        replace upload files with generated files and to actually
> > +        generate the replacement files. We could use std::functions passed
> > +        to FormData instead of calling ChromeClient methods directly.
> 
> This is OK to do, although a better fix would be to stop using the chrome
> client, and use file replacement code from
> BlobDataFileReference::generateReplacementFile(). That would require some
> refactoring, but the code is already in WebCore/platform.

Ah, I didn't know BlobDataFileReference::generateReplacementFile(), I'll rework it then.

> Right now, we potentially archive a bundle twice, first via Blob code path
> if the file is ever accessed via File API, and then via the chrome client if
> the file end up being submitted via a form.
> 
> I did not review the patch in detail.

I'll try to refactor the code to use BlobDataFileReference instead.

[Carlos Garcia Campos]

I've been looking at this in more detail, and it seems to me that the replacement file generations should happen automatically already. The BlobDataFileReference objects, already to the generation on demand when the path() or size() methods are used. The File object also computes the replaced name and content type in its constructor. I can't test this, because the file replacement thing is a mac only feature, but looking at the code I would say that:

 - In FormData::appendKeyValuePairItems, the name and content type we get form the File blob are already computed, so we wouldn't need to use the chrome client, that only computes the name but no the content type. 

  - We don't need to generate the files explicitly, when the Blob files are added to the actual HTTP body by the resource handle, the path() method of BlobDataFileReference will be called to read the file, ensuring the replacement files are generated.

  - And we don't need to explicitly delete the generated files, because the BlobDataFileReference destructor does it already.

I've also noticed that FormData uses FormDataList, that surprisingly it's not in platform but in html. We can probably just move FormDataList to platform, or is there any reason for it to be in html?

[Alexey Proskuryakov]

I think that there is a difference in behavior between FormDataElement::EncodedBlob and FormDataElement::EncodedFile. There is no File object created for form upload unless the file is also accessed via element.files[index]. So, there is no Blob and no BlobDataFileReference.

> We can probably just move FormDataList to platform, or is there any reason for it to be in html?

I'm not aware of any such reason.

[Alexey Proskuryakov]

In other words, I think that this refactoring can be performed on non-Mac by eliminating non-File code path. 

One thing that is challenging about this is that these use modes actually have different semantics now. A file that is put into a form input is stored as a path, so any changes to it that are made between choosing it and submitting the form do take effect. On the other hand, File objects are frozen when the File object is created, and get invalidated after any changes. If such a File object is in a form, submission will suddenly fail. 

This is largely a standards limitation, so we should probably try to preserve existing behavior for now, even though it's crazy.

[Michael Catanzaro]

Carlos, are you still planning on reworking this eventually (in which case please unset r?), or are you hoping to land it as-is?

[Carlos Garcia Campos]

I had forgotten this to be honest. So, maybe we could just fix the layering violations now, and then improve the code instead of the other way around.

[Michael Catanzaro]

Comment on attachment 240599 [details]
Patch

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

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.

> Source/WebCore/loader/FrameLoader.cpp:360
> +    if (Page* page = m_frame.document()->page())

Why do you not need to null-check document and page, you're sure this is safe? My defeatist, non-ideal strategy is to null-check these always, as I've given up on understanding when they can and cannot be null....

> Source/WebCore/loader/FrameLoader.cpp:361
> +        data.generateFiles([page](const String& path) -> String { return page->chrome().client().generateReplacementFile(path); });

Why do you need to specify the return type of the lambda? Is it not inferred?

[Michael Catanzaro]

Ah haha, I had previously left review comments on this patch months ago, but never published the review. :)