Bug 225636 - Allow logging minimal info about sending media files through XMLHttpRequest
Summary: Allow logging minimal info about sending media files through XMLHttpRequest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-10 22:26 PDT by Said Abou-Hallawa
Modified: 2021-05-18 11:48 PDT (History)
10 users (show)

See Also:


Attachments
Patch (7.58 KB, patch)
2021-05-11 00:24 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (7.59 KB, patch)
2021-05-11 00:34 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (7.62 KB, patch)
2021-05-11 11:29 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (8.45 KB, patch)
2021-05-14 20:21 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].