FormData event handler addition
(In reply to Qiaosong Zhou from comment #0) > FormData event handler addition Please provide a better bug title / description. This is way too vague.
Created attachment 432970 [details] Patch
Comment on attachment 432970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432970&action=review Also missing test or test rebaseline. > Source/WebCore/ChangeLog:7 > + Please add detailed change log description. > Source/WebCore/platform/network/FormData.cpp:96 > + scheduleEvent(eventNames().formdataEvent) Spec says we should fire an FormDataEvent, not a generic Event.
Sorry I'm just creating a ChangeLog stub. Haven't finalized the changes yet.
(In reply to Qiaosong Zhou from comment #4) > Sorry I'm just creating a ChangeLog stub. Haven't finalized the changes yet. Then please do not set r? flag, which indicates that you are requesting a review.
Created attachment 433304 [details] Patch
Comment on attachment 433304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433304&action=review > Source/WebCore/dom/FormDataEvent.cpp:43 > + : Event(eventType, initializer, IsTrusted::Yes) This doesn't seem right. Events constructed by JavaScript should NOT be trusted. Only the events constructed by the browser in native code should be trusted. As far as I can tell, the JS constructed ends up calling this constructor and is incorrectly marked as trusted.
Created attachment 433355 [details] Patch
(In reply to Chris Dumez from comment #7) > Comment on attachment 433304 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433304&action=review > > > Source/WebCore/dom/FormDataEvent.cpp:43 > > + : Event(eventType, initializer, IsTrusted::Yes) > > This doesn't seem right. Events constructed by JavaScript should NOT be > trusted. Only the events constructed by the browser in native code should be > trusted. > As far as I can tell, the JS constructed ends up calling this constructor > and is incorrectly marked as trusted. Thanks for the feedback. I'll change this on the next upload. Currently for some reason DOMFormData constructor is called repeatedly when a FormDataEvent is dispatched to HTMLFormElement, causing a stack overflow.
Comment on attachment 433355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433355&action=review > Source/WebCore/html/DOMFormData.cpp:60 > + auto formDataEvent = FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::No, Event::IsCancelable::No, Event::IsComposed::No, this); You probably need to move the event creation / dispatch to DOMFormData::create(), *after* calling the constructor. Calling this from inside the destructor is a bad idea because constructing the FormDataEvent will ref() |this| before it is done constructing.
Created attachment 433367 [details] Patch
Comment on attachment 433367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433367&action=review > Source/WebCore/html/DOMFormData.cpp:64 > + EventDispatcher::dispatchEvent(*form, formDataEvent.get()); form can be null so *form is wrong here.
Comment on attachment 433367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433367&action=review > Source/WebCore/dom/FormDataEvent.h:46 > + static Ref<FormDataEvent> create(const AtomString&, CanBubble, IsCancelable, IsComposed, DOMFormData*); Should take in a Ref<DOMFormData>&&, not a DOMFormData* since it cannot be null. > Source/WebCore/dom/FormDataEvent.h:48 > + DOMFormData* formData() const { return m_formData.get(); } Should return a DOMFormData& since it cannot be null. > Source/WebCore/dom/FormDataEvent.h:53 > + RefPtr<DOMFormData> m_formData; Should use Ref<> since it cannot be null. > Source/WebCore/html/DOMFormData.cpp:63 > + auto formDataEvent = FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::No, Event::IsCancelable::No, Event::IsComposed::No, formData.ptr()); formData.copyRef()
Created attachment 433373 [details] Patch
Comment on attachment 433373 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433373&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:5526 > + FD3160BF12B0272A00C1A359 /* (null) in Headers */ = {isa = PBXBuildFile; }; Suspicious change. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:31622 > + FD3160BF12B0272A00C1A359 /* (null) in Headers */, Ditto. > Source/WebCore/dom/FormDataEvent.h:35 > +struct FormDataEventInit : public EventInit { Should probably be moved inside the FormDataEvent class. > Source/WebCore/dom/FormDataEvent.h:53 > + RefPtr<DOMFormData> m_formData; Please leave a blank line between the functions and the data members. > Source/WebCore/dom/FormDataEvent.idl:28 > + constructor(DOMString type, FormDataEventInit eventInitDict); We use 4 space indentation in WebKit (not 2 space). > Source/WebCore/html/DOMFormData.cpp:61 > + Ref<DOMFormData> formData = adoptRef(*new DOMFormData(form)); auto formData > Source/WebCore/html/DOMFormData.cpp:62 > + auto formDataEvent = FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::No, Event::IsCancelable::No, Event::IsComposed::No, formData.ptr()); Should be inside the if scope below. Why construct an event if you're not going to dispatch it? > Source/WebCore/html/DOMFormData.cpp:64 > + form->dispatchEvent(formDataEvent.get()); form->dispatchEvent(FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::No, Event::IsCancelable::No, Event::IsComposed::No, formData.ptr()));
<rdar://problem/80536177>
Created attachment 433561 [details] Patch
Created attachment 433563 [details] Patch
Created attachment 433565 [details] Patch
Comment on attachment 433565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433565&action=review Note that some tests still need rebaselining and there are a new real-looking crashes as well. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:5527 > + FD3160BF12B0272A00C1A359 /* (null) in Headers */ = {isa = PBXBuildFile; }; Bad change, please drop. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:31611 > + FD3160BF12B0272A00C1A359 /* (null) in Headers */, ditto. > Source/WebCore/dom/FormDataEvent.cpp:38 > + if (!init.formData) Should not be needed. The generated bindings code should already do this check for you, if it is marked as required in the IDL, then you can assume the pointer is non-null here. > Source/WebCore/dom/FormDataEvent.cpp:57 > + , m_formData(formData) WTFMove(formData) to avoid ref-counting churn. > Source/WebCore/dom/FormDataEvent.cpp:66 > +} } // namespace WebCore is our usual pattern. > Source/WebCore/dom/FormDataEvent.h:38 > + struct FormDataEventInit : public EventInit { Just name this struct "Init". Also, no need for "public" since inheritance is public by default for struct (unlike class). > Source/WebCore/dom/FormDataEvent.h:42 > + using Init = FormDataEventInit; Then drop this alias. > Source/WebCore/dom/FormDataEvent.h:44 > + static ExceptionOr<Ref<FormDataEvent>> create(const AtomString&, FormDataEventInit&&); Shouldn't need the ExceptionOr<> > Source/WebCore/dom/FormDataEvent.idl:26 > +[Exposed=Window] The format we usually use in our IDL is: [ Exposed=Window ] interface FormDataEvent : Event { > Source/WebCore/html/DOMFormData.cpp:63 > + return dispatchFormDataEvent(adoptRef(*new DOMFormData(form)), *form); Why aren't you using DOMFormData::create(form) ? Also, if we should to leave the cloning up to the caller, then it would look like: if (form) { auto formData = DOMFormData::create(form); auto dispatchResult = dispatchFormDataEvent(formData, *form); if (dispatchResult.hasException()) return dispatchResult.releaseException(); return formData->clone(); } > Source/WebCore/html/DOMFormData.cpp:71 > + if (!DOMFormData::formDataDispatched) { Seems like this should be an early return instead: if (isDispatchingFormDataEvent) return Exception { InvalidStateError, "Cannot call \"new FormData()\" within FormDataEvent handler"_s }; > Source/WebCore/html/DOMFormData.cpp:77 > + return DOMFormData::create(WTFMove(formData)); Given that not all call sites seem to want cloning, we should probably return { }; return and have the return type be ExceptionOr<void>. > Source/WebCore/html/DOMFormData.cpp:88 > +Ref<DOMFormData> DOMFormData::create(Ref<DOMFormData>&& other) It feels weird to have a factory function on DOMFormData that takes in a Ref<DOMFormData>&& in. Shouldn't we simply add a "clone()" member function to DOMFormData? > Source/WebCore/html/DOMFormData.h:58 > + inline static bool formDataDispatched = false; No inline here. I don't think using a static variable makes sense either. Imagine 2 tabs sharing the same process, I don't think you'd want the formdata dispatch for one page to be impacted by the formdata dispatch in another. Also, the name should be something like "isDispatchingFormDataEvent". I am also unclear why this is public. > Source/WebCore/loader/FormSubmission.cpp:230 > + DOMFormData::dispatchFormDataEvent(domFormData.copyRef(), form); How come we're not using the returned DOMFormData here? If this call site doesn't need to clone, then dispatchFormDataEvent() should probably not do the cloning and we should leave it up to the caller.
Comment on attachment 433565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433565&action=review > Source/WebCore/dom/GlobalEventHandlers.idl:59 > // FIXME: Implement 'onformdata'. This line should be removed.
Comment on attachment 433565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433565&action=review >> Source/WebCore/html/DOMFormData.h:58 >> + inline static bool formDataDispatched = false; > > No inline here. I don't think using a static variable makes sense either. Imagine 2 tabs sharing the same process, I don't think you'd want the formdata dispatch for one page to be impacted by the formdata dispatch in another. > > Also, the name should be something like "isDispatchingFormDataEvent". > > I am also unclear why this is public. This could be a function-scoped static bool instead of a class-scoped static bool because it is only used in one function.
(In reply to Alex Christensen from comment #22) > Comment on attachment 433565 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433565&action=review > > >> Source/WebCore/html/DOMFormData.h:58 > >> + inline static bool formDataDispatched = false; > > > > No inline here. I don't think using a static variable makes sense either. Imagine 2 tabs sharing the same process, I don't think you'd want the formdata dispatch for one page to be impacted by the formdata dispatch in another. > > > > Also, the name should be something like "isDispatchingFormDataEvent". > > > > I am also unclear why this is public. > > This could be a function-scoped static bool instead of a class-scoped static > bool because it is only used in one function. As I said, it should not be a static at all. It doesn't make sense.
(In reply to Chris Dumez from comment #23) > As I said, it should not be a static at all. It doesn't make sense. I have removed the static variable. Instead I'm using a threadGlobalData member variable to address the problem of threads sharing static variables. I hope that's the optimal solution here?
(In reply to Chris Dumez from comment #20) > > Source/WebCore/html/DOMFormData.cpp:63 > > + return dispatchFormDataEvent(adoptRef(*new DOMFormData(form)), *form); > > Why aren't you using DOMFormData::create(form) ? > > Also, if we should to leave the cloning up to the caller, then it would look > like: > if (form) { > auto formData = DOMFormData::create(form); > auto dispatchResult = dispatchFormDataEvent(formData, *form); > if (dispatchResult.hasException()) > return dispatchResult.releaseException(); > return formData->clone(); > } > > > Source/WebCore/html/DOMFormData.cpp:77 > > + return DOMFormData::create(WTFMove(formData)); > > Given that not all call sites seem to want cloning, we should probably > return { }; return and have the return type be ExceptionOr<void>. The reason that I have to clone for in this case is that if the handler for FormDataEvent stores the DOMFormData object in the outer scope, the correct behavior is that changing the outer scope reference outside of the event handler should not change the original DOMFormData object. To fall in line with that, I clone the DOMFormData after the event is dispatched. > > Source/WebCore/html/DOMFormData.cpp:88 > > +Ref<DOMFormData> DOMFormData::create(Ref<DOMFormData>&& other) > > It feels weird to have a factory function on DOMFormData that takes in a > Ref<DOMFormData>&& in. Shouldn't we simply add a "clone()" member function > to DOMFormData? Yes I will apply that change. > > > Source/WebCore/loader/FormSubmission.cpp:230 > > + DOMFormData::dispatchFormDataEvent(domFormData.copyRef(), form); > > How come we're not using the returned DOMFormData here? > > If this call site doesn't need to clone, then dispatchFormDataEvent() should > probably not do the cloning and we should leave it up to the caller. Same as above, I feel like cloning should be mandatory to prevent illegal tampering with DOMFormData from outside of the event handler.
Specifically, the last two asserts for test "Newly created FormData contains entries added to "formData" IDL attribute of FormDataEvent." on http://wpt.live/xhr/formdata.html seems to mandate cloning.
*** Bug 193231 has been marked as a duplicate of this bug. ***
Created attachment 433772 [details] Patch
Created attachment 433773 [details] Patch
Created attachment 433775 [details] Patch
Created attachment 433782 [details] Patch
Comment on attachment 433782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433782&action=review > Source/WebCore/html/DOMFormData.cpp:71 > + if (threadGlobalData().isFormDataEventDispatched()) This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions. > Source/WebCore/platform/network/FormData.cpp:278 > + // FIXME: Rmove the following if statement when fixed. Typo: Remove
Comment on attachment 433782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433782&action=review > Source/WebCore/html/DOMFormData.cpp:92 > + for (auto& item : this->items()) Any reason we cannot simply do: ``` newFormData->m_items = m_items; ``` ?
Comment on attachment 433782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433782&action=review >> Source/WebCore/html/DOMFormData.cpp:71 >> + if (threadGlobalData().isFormDataEventDispatched()) > > This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions. We likely want a flag on the HTMLFormElement or FormSubmission instead.
Comment on attachment 433782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433782&action=review >>> Source/WebCore/html/DOMFormData.cpp:71 >>> + if (threadGlobalData().isFormDataEventDispatched()) >> >> This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions. > > We likely want a flag on the HTMLFormElement or FormSubmission instead. It looks like this needs to move to DOMFormData::create, but I don't think this can be accomplished with a flag on HTMLFormElement or FormSubmission. This needs to change what DOMFormData::create returns if it is called inside the stack trace of dispatching the FormDataEvent.
Comment on attachment 433782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433782&action=review >>>> Source/WebCore/html/DOMFormData.cpp:71 >>>> + if (threadGlobalData().isFormDataEventDispatched()) >>> >>> This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions. >> >> We likely want a flag on the HTMLFormElement or FormSubmission instead. > > It looks like this needs to move to DOMFormData::create, but I don't think this can be accomplished with a flag on HTMLFormElement or FormSubmission. This needs to change what DOMFormData::create returns if it is called inside the stack trace of dispatching the FormDataEvent. The form is passed as parameter here so I don't see why a flag on the form wouldn't work. Note that as far as I can tell, Blink is using a flag on the form.
Comment on attachment 433782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433782&action=review >>>>> Source/WebCore/html/DOMFormData.cpp:71 >>>>> + if (threadGlobalData().isFormDataEventDispatched()) >>>> >>>> This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions. >>> >>> We likely want a flag on the HTMLFormElement or FormSubmission instead. >> >> It looks like this needs to move to DOMFormData::create, but I don't think this can be accomplished with a flag on HTMLFormElement or FormSubmission. This needs to change what DOMFormData::create returns if it is called inside the stack trace of dispatching the FormDataEvent. > > The form is passed as parameter here so I don't see why a flag on the form wouldn't work. Note that as far as I can tell, Blink is using a flag on the form. For the record, this is the section of the spec where this is specified: https://html.spec.whatwg.org/#constructing-form-data-set:formdataevent "Set form's firing submission events to true." / "Set form's firing submission events to false." In particular this flag: https://html.spec.whatwg.org/#constructing-entry-list Which is a flag on the form. Please match the spec exactly.
Oh, I see. Not all FormData constructors will fail inside of that event, just the ones using that FormData with the flag. You're right.
Comment on attachment 433782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433782&action=review >>>>>> Source/WebCore/html/DOMFormData.cpp:71 >>>>>> + if (threadGlobalData().isFormDataEventDispatched()) >>>>> >>>>> This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions. >>>> >>>> We likely want a flag on the HTMLFormElement or FormSubmission instead. >>> >>> It looks like this needs to move to DOMFormData::create, but I don't think this can be accomplished with a flag on HTMLFormElement or FormSubmission. This needs to change what DOMFormData::create returns if it is called inside the stack trace of dispatching the FormDataEvent. >> >> The form is passed as parameter here so I don't see why a flag on the form wouldn't work. Note that as far as I can tell, Blink is using a flag on the form. > > For the record, this is the section of the spec where this is specified: > https://html.spec.whatwg.org/#constructing-form-data-set:formdataevent > "Set form's firing submission events to true." / "Set form's firing submission events to false." > > In particular this flag: > https://html.spec.whatwg.org/#constructing-entry-list > > Which is a flag on the form. > > Please match the spec exactly. And from the spec, it looks like this flag is being checked when submitting a form, not when constructing a DOMFormData, so this is definitely the wrong place. See https://html.spec.whatwg.org/#form-submission-algorithm:constructing-entry-list
Created attachment 433817 [details] Patch
Comment on attachment 433817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433817&action=review > Source/WebCore/html/DOMFormData.cpp:71 > + if (form.formDataEventDispatched()) As mentioned earlier, this is not the right place to check this. The name of the flag is also wrong. Please match the spec text.
(In reply to Chris Dumez from comment #41) > Comment on attachment 433817 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433817&action=review > > > Source/WebCore/html/DOMFormData.cpp:71 > > + if (form.formDataEventDispatched()) > > As mentioned earlier, this is not the right place to check this. The name of > the flag is also wrong. Please match the spec text. Looking at the spec for the FormData constructor here: - https://xhr.spec.whatwg.org/#dom-formdata It does appear that it calls "construct an entry-list" (https://html.spec.whatwg.org/#constructing-entry-list), which does check the flag. So I think we need to try and match the spec a bit more closely for clarity. Ideally, we'd have a constructEntryList() function which is called both from the FormData constructor and form submission and this function would return early if the flag is set. And we would rename the flag to match the spec more exactly. While the behavior now is properly more or less correct, it will be much more obvious once our implementation matches for directly the spec language.
Created attachment 433833 [details] Patch
Created attachment 433845 [details] Patch
Created attachment 433882 [details] Patch
Created attachment 433894 [details] Patch
Created attachment 433895 [details] Patch
Comment on attachment 433895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433895&action=review > Source/WebCore/html/DOMFormData.cpp:71 > + if (form.formDataEventDispatched()) Still not matching the spec. There is no such flag in the spec, as mentioned earlier. There is a flag in the spec but it is not about the form data event being dispatched, it is about constructing an entry list.
Created attachment 433980 [details] Patch
Comment on attachment 433980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433980&action=review > Source/WebCore/html/DOMFormData.cpp:59 > + May be good to have a link to the corresponding spec section here as a comment: // https://html.spec.whatwg.org/C/#constructing-the-form-data-set > Source/WebCore/html/DOMFormData.cpp:60 > +ExceptionOr<Ref<DOMFormData>> DOMFormData::constructEntryList(HTMLFormElement& form, StringPairVector* formValues, bool isMultiPartForm, bool& containsPasswordData) Could we move this to HTMLFormElement ? This way, we don't need to pass in a form. Also, we probably won't need the public isConstructingEntryList() / setIsConstructingEntryList() on HTMLFormElement. > Source/WebCore/html/DOMFormData.cpp:64 > + I think this function is supposed to: 1. Construct a new DOMFormData object 2. Do your loop below which populates the DOMFormData object 3. dispatch the FormDataEvent 4. Return the DOMFormData object > Source/WebCore/html/DOMFormData.cpp:65 > + form.setIsConstructingEntryList(true); Once you move this to HTMLFormElement, please use WTF::SetForScope to change the value of m_isConstructingEntryList. > Source/WebCore/html/DOMFormData.cpp:97 > +void DOMFormData::dispatchFormDataEvent(Ref<DOMFormData>&& formData, HTMLFormElement& form) We probably don't need a whole function for this. We can probably just merge into constructEntryList() as a one liner: dispatchEvent(FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::Yes, Event::IsCancelable::No, Event::IsComposed::No, formData.copyRef())); > Source/WebCore/html/DOMFormData.cpp:113 > + Unnecessary blank line. > Source/WebCore/html/DOMFormData.h:56 > + static void dispatchFormDataEvent(Ref<DOMFormData>&&, HTMLFormElement&); Not needed. > Source/WebCore/html/HTMLFormElement.cpp:402 > + auto result = FormSubmission::create(*this, submitter, m_attributes, event, shouldLockHistory, trigger); I don't think HTMLFormElement::submit() is supposed to throw an exception. I also don't think that it is possible that the isConstructingEntryList flag is true. I think constructEntryListList() should just return a RefPtr<> and let the call-site decide if they want to throw or not. In the DOMFormData constructor case, we'd throw if constructEntryList() returns null. In the form submission case, we'd RELEASE_ASSERT() that the value is non-null. > Source/WebCore/loader/FormSubmission.cpp:217 > + if (entryListResult.hasException()) { I don't think this case is possible, I think we should just RELEASE_ASSERT() that constructEntryList returned a valid domFormData.
Comment on attachment 433980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433980&action=review > Source/WebCore/html/DOMFormData.cpp:63 > + return Exception { InvalidStateError, "Already constructing Form entry list."_s }; I think we should just return null here, like in the spec text and let the DOMFormData constructor throw the exception if it get null. >> Source/WebCore/html/DOMFormData.cpp:64 >> + > > I think this function is supposed to: > 1. Construct a new DOMFormData object > 2. Do your loop below which populates the DOMFormData object > 3. dispatch the FormDataEvent > 4. Return the DOMFormData object To clarify, for step 4, I meant return a *clone* of the DOMFormData, as per https://html.spec.whatwg.org/C/#constructing-the-form-data-set
Created attachment 434044 [details] Patch
Created attachment 434057 [details] Patch
Created attachment 434080 [details] Patch
Comment on attachment 434080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434080&action=review Looks like you addressed Chris's comments. I think it looks good. A few style things and I'd say it's ready. > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). nit: space below > Source/WebCore/html/DOMFormData.cpp:36 > +#include "ThreadGlobalData.h" This isn't used any more. > Source/WebCore/html/HTMLFormElement.h:128 > + RefPtr<DOMFormData> constructEntryList(Ref<DOMFormData>&&, StringPairVector*, bool, bool&); Style nit: It would be nicer if this didn't have an out-param and used 2-state enums instead of bools to make the call sites more readable. Like this: enum class IsMultiPartForm : bool { No, Yes }; enum class ContainsPasswordData : bool { No, Yes }; std::pair<RefPtr<DOMFormData>, ContainsPasswordData> constructEntryList(Ref<DOMFormData>&&, StringPairVector*, IsMultiPartForm);
Comment on attachment 434080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434080&action=review > Source/WebCore/html/DOMFormData.cpp:52 > + auto result = form->constructEntryList(WTFMove(domFormData), nullptr, true, temp); Why is it correct to unconditionally pass isMultiPartForm=true here? I might be, I just don't know why. > Source/WebCore/html/HTMLFormElement.cpp:973 > + SetForScope<bool> isConstructingEntryListRestorer(m_isConstructingEntryList, true); I'd name this: "isConstructingEntryListScope". We usually use "Scope" in the name for such things. > Source/WebCore/loader/FormSubmission.cpp:215 > + auto result = form.constructEntryList(WTFMove(domFormData), &formValues, isMultiPartForm, containsPasswordData); Could constructEntryList() call fomFormData->setContainsPasswordData() by itself instead of requiring an out-parameter?
Comment on attachment 434080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434080&action=review > Source/WebCore/html/DOMFormData.cpp:51 > + bool temp; FYI, temp is a terrible name and I would have used to actual parameter name here. That said, I think we should be able to get rid of this out-parameter entirely with my earlier suggestion. >> Source/WebCore/html/DOMFormData.cpp:52 >> + auto result = form->constructEntryList(WTFMove(domFormData), nullptr, true, temp); > > Why is it correct to unconditionally pass isMultiPartForm=true here? I might be, I just don't know why. It is probably fine. Looks like it is only used by FileInputType::appendFormData() for something that is likely obsolete. Passing true is likely OK but I'd switch to an enum class for clarity, like Alex mentioned.
Created attachment 434114 [details] Patch
Created attachment 434115 [details] Patch
Comment on attachment 434115 [details] Patch Looks good to me. I'll let Chis double check it.
Comment on attachment 434115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434115&action=review > Source/WebCore/html/HTMLFormElement.h:132 > + bool containsPasswordData() const { return m_containsPasswordData; } As far as I can tell, nobody ever consults the value of containsPasswordData. (And, the value is lost accidentally when serializing across processes in FormData::encode(), and when serializing across threads in FormData::isolatedCopy(), consistent with a theory that it may be vestigial and unused.) I think you can remove containsPasswordData, and the associated code you need to modify in order to maintain it. Why don't you give that a shot and see if it builds. > Source/WebCore/platform/network/FormData.cpp:278 > + // FIXME: DOMFormData created in FormData Event callback with text/plain enctype will crash here because values can be file. The expected behavior is to convert files to string for enctype "text/plain". Conversion may be added at "void DOMFormData::set(const String& name, Blob& blob, const String& filename)" or here. > + // FIXME: Remove the following if statement when fixed. I would say just: // FIXME: if the item is a file, we are expected to converted the file to a string. No need to mention that a crash is how you started debugging; what you've learned since then is most helpful to the next reader.
Created attachment 434149 [details] Patch
Comment on attachment 434149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434149&action=review r=me with one comment. > Source/WebCore/platform/network/FormData.cpp:278 > + if (!WTF::holds_alternative<String>(item.data)) Is this really still needed? I thought my proposal to do `newFormData->m_items = m_items;` would avoid this.
(In reply to Chris Dumez from comment #63) > Comment on attachment 434149 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434149&action=review > > r=me with one comment. > > > Source/WebCore/platform/network/FormData.cpp:278 > > + if (!WTF::holds_alternative<String>(item.data)) > > Is this really still needed? I thought my proposal to do > `newFormData->m_items = m_items;` would avoid this. It is still needed. It is related to another issue, where the current formdata does not process forms with enctype="text/plain" properly. It's supposed to automatically convert files to string if the form is "text/plain". Currently it is possible to receive a file here, causing a crash when the variant is unparsed as a string. (further down the line) The specific test case that causes a crash is http://wpt.live/html/semantics/forms/form-submission-0/text-plain.window.html
(In reply to Qiaosong Zhou from comment #64) > (In reply to Chris Dumez from comment #63) > > Comment on attachment 434149 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=434149&action=review > > > > r=me with one comment. > > > > > Source/WebCore/platform/network/FormData.cpp:278 > > > + if (!WTF::holds_alternative<String>(item.data)) > > > > Is this really still needed? I thought my proposal to do > > `newFormData->m_items = m_items;` would avoid this. > > It is still needed. It is related to another issue, where the current > formdata does not process forms with enctype="text/plain" properly. It's > supposed to automatically convert files to string if the form is > "text/plain". Currently it is possible to receive a file here, causing a > crash when the variant is unparsed as a string. (further down the line) > > The specific test case that causes a crash is > http://wpt.live/html/semantics/forms/form-submission-0/text-plain.window.html Ok.
Committed r280310 (239960@main): <https://commits.webkit.org/239960@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434149 [details].
Comment on attachment 434149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434149&action=review > Source/WebCore/dom/FormDataEvent.h:45 > + DOMFormData& formData() const { return m_formData.get(); } Seems like this should return a const& for conceptual const-ness. > Source/WebCore/html/DOMFormData.cpp:46 > + auto domFormData = adoptRef(*new DOMFormData()); I think the local variable should just be named formData or data. Also no need for the (). > Source/WebCore/html/DOMFormData.cpp:48 > + return domFormData; This should have a WTFMove() because the return value optimization doesn’t work if we don’t have types that match exactly. > Source/WebCore/html/DOMFormData.h:33 > +#include "ExceptionOr.h" Should not need to include this. A forward declaration should be sufficient. > Source/WebCore/html/DOMFormData.h:35 > +#include "FormState.h" Should not need to include this. Not clear why it’s needed. > Source/WebCore/html/DOMFormData.h:68 > + Ref<DOMFormData> clone(); Should be const.
Reopening to attach new patch.
Created attachment 434254 [details] Patch
Comment on attachment 434254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434254&action=review > Source/WebCore/html/DOMFormData.cpp:46 > + auto data = adoptRef(*new DOMFormData); I'd much prefer "formData" to the overly generic "data" here.
Created attachment 434255 [details] Patch
Committed r280332 (239979@main): <https://commits.webkit.org/239979@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434255 [details].
Comment on attachment 434149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434149&action=review >> Source/WebCore/html/DOMFormData.cpp:48 >> + return domFormData; > > This should have a WTFMove() because the return value optimization doesn’t work if we don’t have types that match exactly. According to r280376, I was wrong about this. I would like to find out more about why I was wrong!
(In reply to Darin Adler from comment #73) > Comment on attachment 434149 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434149&action=review > > >> Source/WebCore/html/DOMFormData.cpp:48 > >> + return domFormData; > > > > This should have a WTFMove() because the return value optimization doesn’t work if we don’t have types that match exactly. > > According to r280376, I was wrong about this. I would like to find out more > about why I was wrong! I think return optimization implicitly moves the rvalue. This link explains it better: https://stackoverflow.com/questions/17473753/c11-return-value-optimization-or-move