Bug 38397 - Add "preSubmitForm" hook to FrameLoader
: Add "preSubmitForm" hook to FrameLoader
Status: RESOLVED FIXED
: WebKit
WebKit API
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-04-30 12:35 PST by
Modified: 2010-05-03 11:12 PST (History)


Attachments
patch (6.43 KB, patch)
2010-04-30 12:51 PST, Jens Alfke
fishd: review-
Review Patch | Details | Formatted Diff | Diff
patch 2 (5.46 KB, patch)
2010-04-30 14:03 PST, Jens Alfke
no flags Review Patch | Details | Formatted Diff | Diff
patch 3 (15.03 KB, patch)
2010-04-30 16:54 PST, Jens Alfke
fishd: review+
Review Patch | Details | Formatted Diff | Diff
patch 4 (16.06 KB, patch)
2010-05-03 09:45 PST, Jens Alfke
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-30 12:35:26 PST
In some cases* the browser may need to examine the contents of a form immediately before it's submitted — in particular, before any of the page's onSubmit handlers have had a chance to run. WebKit does not currently have any hook for this, since the existing "willSubmitForm" delegate method in the API does not get called until after the form submission has actually begun.

The enclosed patch remedies this by adding a "dispatchPreSubmitForm" method to the WebCore::FrameLoaderClient interface. (It's not abstract: it defaults to doing nothing. I did this because it won't always be needed, and adding an abstract method to FrameLoaderClient requires modifying half a dozen subclasses for at least four different OSs, to avoid breaking the build.)

I plumbed this through Chromium's WebKit API, because I need this callback for some work I'm doing on Chrome. I didn't touch the APIs for other platforms, but it would of course be easy to add if anyone else needs this functionality.

[* The case in question is <http://code.google.com/p/chromium/issues/detail?id=28910>. Some websites modify the login form, clearing the password field and putting a hashed password into a hidden field, in the onSubmit handler. This breaks Chrome's (and Safari's) password autofill, because by the time it registers that the form was submitted, it's too late to capture the password. The autofill code needs a chance to look at the form before the onSubmit handler runs.]
------- Comment #1 From 2010-04-30 12:51:27 PST -------
Created an attachment (id=54819) [details]
patch
------- Comment #2 From 2010-04-30 13:01:43 PST -------
(From update of attachment 54819 [details])
WebCore/html/HTMLFormElement.cpp:294
 +      frame->loader()->dispatchPreSubmitForm(this);
I would just do frame->loader()->client()->dispatch* here instead
of adding the intermediate FrameLoader method.

WebCore/loader/FrameLoaderClient.h:149
 +          virtual void dispatchPreSubmitForm(HTMLFormElement*) { };
How about naming this dispatchWillSendSubmitEvent?  I think that name is more accurate.

WebKit/chromium/public/WebFrameClient.h:143
 +      virtual void preSubmitForm(WebFrame*, const WebFormElement&) { }
willSendSubmitEvent
------- Comment #3 From 2010-04-30 14:03:12 PST -------
Created an attachment (id=54826) [details]
patch 2

Sounds good. Here's an updated patch.
------- Comment #4 From 2010-04-30 15:38:19 PST -------
(From update of attachment 54826 [details])
WebCore/loader/FrameLoaderClient.h:149
> +          virtual void dispatchWillSendSubmitEvent(HTMLFormElement*) { };
no trailing suffix.  actually, this should be pure, and the numerous
implementations of this interface should be updated to have an empty
method.  that way if the method name is changed, it will be easy to
tell if it impacts any ports.

we don't use that rule in WebFrameClient.h since we have different
constraints there:  we are optimizing there for avoiding painful
two-sided commits.

WebKit/chromium/src/FrameLoaderClientImpl.cpp:955
> +  void FrameLoaderClientImpl::dispatchWillSendSubmitEvent(WebCore::HTMLFormElement* form)
no need for WebCore:: prefix in this file.  there is
a 'using namespace WebCore' at the top of the file.

WebKit/chromium/public/WebFrameClient.h:141
> +      // A form submission has been requested, but the page's onSubmit handlers
nit: onSubmit -> submit event
------- Comment #5 From 2010-04-30 16:54:17 PST -------
Created an attachment (id=54836) [details]
patch 3

OK, made the new method abstract. That unfortunately required touching about 8 more files (all the places it's implemented across all platforms.)
------- Comment #6 From 2010-04-30 19:38:45 PST -------
Is the patch missing a change for the win port?
------- Comment #7 From 2010-05-01 00:21:35 PST -------
> Some websites
> modify the login form, clearing the password field and putting a hashed
> password into a hidden field, in the onSubmit handler.

Is it known why those sites are doing that? Is the purpose to prevent autofill?
------- Comment #8 From 2010-05-01 13:31:55 PST -------
(In reply to comment #7)
> > Some websites
> > modify the login form, clearing the password field and putting a hashed
> > password into a hidden field, in the onSubmit handler.
> 
> Is it known why those sites are doing that? Is the purpose to prevent autofill?

I don't think we know for sure, but it seems that they would just use autocomplete=off if they cared about defeating password managers.
------- Comment #9 From 2010-05-02 23:12:11 PST -------
> Is it known why those sites are doing that? Is the purpose to prevent autofill?

No, the purpose is to avoid sending the password in the clear, without having to implement SSL on the server. I think LiveJournal does it because SSL used to be too expensive, and SimpleMachines does it because they want their forum software to be able to run in a typical hosted web setup.
------- Comment #10 From 2010-05-03 09:45:38 PST -------
Created an attachment (id=54933) [details]
patch 4

Added the new method to Windows WebFrame class.
------- Comment #11 From 2010-05-03 11:12:50 PST -------
(From update of attachment 54933 [details])
Clearing flags on attachment: 54933

Committed r58686: <http://trac.webkit.org/changeset/58686>
------- Comment #12 From 2010-05-03 11:12:57 PST -------
All reviewed patches have been landed.  Closing bug.