Summary: | Constructing a FormData from a form can lead to entries with lone surrogates | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreu Botella <andreu> | ||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, cdumez, changseok, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, mehmetgelisin, mifenton, webkit-bug-importer, wenson_hsieh | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Andreu Botella
2021-05-03 05:20:26 PDT
With https://github.com/whatwg/html/pull/7371, the fact that entry names and values which are not File objects are scalar value strings will now become an invariant of form entries. Created attachment 445352 [details]
Patch
Comment on attachment 445352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445352&action=review r=me with comments. > Source/WebCore/html/DOMFormData.cpp:81 > + return { usvName, usvValue }; We could write this in a more concise way: return { replaceUnpairedSurrogatesWithReplacementCharacter(String { usvName }), replaceUnpairedSurrogatesWithReplacementCharacter(String { usvValue }) }; > Source/WebCore/html/DOMFormData.cpp:88 > + usvName = replaceUnpairedSurrogatesWithReplacementCharacter(WTFMove(usvName)); I think this would look a bit nicer: auto usvName = replaceUnpairedSurrogatesWithReplacementCharacter(String { usvName }); > Source/WebCore/html/DOMFormData.h:83 > + Item createStringEntry(const String& name, const String& value); It seems this can be static. Also, since it is private, no need to declare it in the header. Comment on attachment 445352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445352&action=review >> Source/WebCore/html/DOMFormData.cpp:81 >> + return { usvName, usvValue }; > > We could write this in a more concise way: > > return { > replaceUnpairedSurrogatesWithReplacementCharacter(String { usvName }), > replaceUnpairedSurrogatesWithReplacementCharacter(String { usvValue }) > }; I still don't fully understand how && and std::move work, and I didn't realize you could write it that way. >> Source/WebCore/html/DOMFormData.h:83 >> + Item createStringEntry(const String& name, const String& value); > > It seems this can be static. Also, since it is private, no need to declare it in the header. I'll also be removing createFileEntry from the header then, for consistency. (In reply to Andreu Botella from comment #6) > Comment on attachment 445352 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=445352&action=review > > >> Source/WebCore/html/DOMFormData.cpp:81 > >> + return { usvName, usvValue }; > > > > We could write this in a more concise way: > > > > return { > > replaceUnpairedSurrogatesWithReplacementCharacter(String { usvName }), > > replaceUnpairedSurrogatesWithReplacementCharacter(String { usvValue }) > > }; > > I still don't fully understand how && and std::move work, and I didn't > realize you could write it that way. > > >> Source/WebCore/html/DOMFormData.h:83 > >> + Item createStringEntry(const String& name, const String& value); > > > > It seems this can be static. Also, since it is private, no need to declare it in the header. > > I'll also be removing createFileEntry from the header then, for consistency. Yes, please. Created attachment 445423 [details]
Patch
Comment on attachment 445423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445423&action=review > Source/WebCore/html/DOMFormData.cpp:73 > +auto createStringEntry(const String& name, const String& value) -> DOMFormData::Item Please mark as static > Source/WebCore/html/DOMFormData.cpp:82 > +auto createFileEntry(const String& name, Blob& blob, const String& filename) -> DOMFormData::Item ditto. Created attachment 445426 [details]
Patch
Committed r286310 (244669@main): <https://commits.webkit.org/244669@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445426 [details]. Comment on attachment 445426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445426&action=review > Source/WebCore/html/DOMFormData.cpp:87 > + return { usvName, File::create(blob.scriptExecutionContext(), blob, filename.isNull() ? "blob"_s : filename) }; I believe we can slightly reduce reference count churn by using WTFMove(usvName) here. > Source/WebCore/html/DOMFormData.cpp:90 > + return { usvName, File::create(blob.scriptExecutionContext(), downcast<File>(blob), filename) }; Ditto. > Source/WebCore/html/DOMFormData.cpp:92 > + return { usvName, RefPtr<File> { &downcast<File>(blob) } }; Ditto. One other thought: I think we can just write RefPtr instead of RefPtr<File>. |