WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
38397
Add "preSubmitForm" hook to FrameLoader
https://bugs.webkit.org/show_bug.cgi?id=38397
Summary
Add "preSubmitForm" hook to FrameLoader
Jens Alfke
Reported
2010-04-30 12:35:26 PDT
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.]
Attachments
patch
(6.43 KB, patch)
2010-04-30 12:51 PDT
,
Jens Alfke
fishd
: review-
Details
Formatted Diff
Diff
patch 2
(5.46 KB, patch)
2010-04-30 14:03 PDT
,
Jens Alfke
no flags
Details
Formatted Diff
Diff
patch 3
(15.03 KB, patch)
2010-04-30 16:54 PDT
,
Jens Alfke
fishd
: review+
Details
Formatted Diff
Diff
patch 4
(16.06 KB, patch)
2010-05-03 09:45 PDT
,
Jens Alfke
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Jens Alfke
Comment 1
2010-04-30 12:51:27 PDT
Created
attachment 54819
[details]
patch
Darin Fisher (:fishd, Google)
Comment 2
2010-04-30 13:01:43 PDT
Comment on
attachment 54819
[details]
patch 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
Jens Alfke
Comment 3
2010-04-30 14:03:12 PDT
Created
attachment 54826
[details]
patch 2 Sounds good. Here's an updated patch.
Darin Fisher (:fishd, Google)
Comment 4
2010-04-30 15:38:19 PDT
Comment on
attachment 54826
[details]
patch 2 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
Jens Alfke
Comment 5
2010-04-30 16:54:17 PDT
Created
attachment 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.)
Darin Fisher (:fishd, Google)
Comment 6
2010-04-30 19:38:45 PDT
Is the patch missing a change for the win port?
Alexey Proskuryakov
Comment 7
2010-05-01 00:21:35 PDT
> 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?
Darin Fisher (:fishd, Google)
Comment 8
2010-05-01 13:31:55 PDT
(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.
Jens Alfke
Comment 9
2010-05-02 23:12:11 PDT
> 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.
Jens Alfke
Comment 10
2010-05-03 09:45:38 PDT
Created
attachment 54933
[details]
patch 4 Added the new method to Windows WebFrame class.
WebKit Commit Bot
Comment 11
2010-05-03 11:12:50 PDT
Comment on
attachment 54933
[details]
patch 4 Clearing flags on attachment: 54933 Committed
r58686
: <
http://trac.webkit.org/changeset/58686
>
WebKit Commit Bot
Comment 12
2010-05-03 11:12:57 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug