Bug 225636

Summary: Allow logging minimal info about sending media files through XMLHttpRequest
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, changseok, esprehn+autocc, ews-watchlist, gyuyoung.kim, mifenton, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=223706
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2021-05-10 22:26:57 PDT
Some files may be uploaded from JavaScript through XMLHttpRequest. We need to handle this case as we did for HTMLFormElement::submit in r275103.
Comment 1 Said Abou-Hallawa 2021-05-11 00:23:17 PDT
<rdar://problem/76639138>
Comment 2 Said Abou-Hallawa 2021-05-11 00:24:09 PDT
Created attachment 428252 [details]
Patch
Comment 3 Said Abou-Hallawa 2021-05-11 00:34:31 PDT
Created attachment 428254 [details]
Patch
Comment 4 Said Abou-Hallawa 2021-05-11 11:29:17 PDT
Created attachment 428302 [details]
Patch
Comment 5 Alex Christensen 2021-05-12 11:24:34 PDT
Comment on attachment 428302 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:408
> +    document().page()->logSubmittedImageOrMediaFiles(imageOrMediaFilesCount);

Let's null check page:
if (auto* page = document().page())
    page->...

> Source/WebCore/page/Page.cpp:1056
> +void Page::logSubmittedImageOrMediaFiles(unsigned imageOrMediaFilesCount)

Why don't we just make one function and pass a FormData& as a parameter instead of one function to count then another function to log?

> Source/WebCore/xml/XMLHttpRequest.cpp:549
> +        if (auto page = scriptExecutionContext()->isDocument() ? document()->page() : nullptr)

auto*

It's also not great to introduce code in XHR that only works in document context.  This fixes another case of unreported FormData uploading, but workers are the next case and fetch is probably the one after that.  This might be nicer in a more central location, like if we added a request parameter to addParametersShared and checked its HTTP body there, instead of one in HTMLFormElement and one in xhr.
Comment 6 Said Abou-Hallawa 2021-05-14 20:21:07 PDT
Created attachment 428703 [details]
Patch
Comment 7 Said Abou-Hallawa 2021-05-15 13:44:37 PDT
Comment on attachment 428302 [details]
Patch

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

>> Source/WebCore/xml/XMLHttpRequest.cpp:549
>> +        if (auto page = scriptExecutionContext()->isDocument() ? document()->page() : nullptr)
> 
> auto*
> 
> It's also not great to introduce code in XHR that only works in document context.  This fixes another case of unreported FormData uploading, but workers are the next case and fetch is probably the one after that.  This might be nicer in a more central location, like if we added a request parameter to addParametersShared and checked its HTTP body there, instead of one in HTMLFormElement and one in xhr.

This is a great idea. I was looking for a common place for all file uploading but I could not find one because I was looking for it in WebCore. I added the logging in addParametersShared().
Comment 8 EWS 2021-05-18 11:48:56 PDT
Committed r277666 (237868@main): <https://commits.webkit.org/237868@main>

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