Bug 38765 - [Chromium] WebFrame::registerPasswordListener shouldn't assert on duplicate listener
: [Chromium] WebFrame::registerPasswordListener shouldn't assert on duplicate l...
Product: WebKit
Classification: Unclassified
Component: WebKit API
: 528+ (Nightly build)
: All All
: P3 Minor
Assigned To: Jens Alfke
Depends on:
  Show dependency treegraph
Reported: 2010-05-07 12:13 PDT by Jens Alfke
Modified: 2010-05-08 11:06 PDT (History)
3 users (show)

See Also:

patch (3.21 KB, patch)
2010-05-07 12:19 PDT, Jens Alfke
no flags Details | Formatted Diff | Diff
patch 2 (3.21 KB, patch)
2010-05-07 13:17 PDT, Jens Alfke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jens Alfke 2010-05-07 12:13:20 PDT
The Chromium WebKit API's implementation of WebFrame::registerPasswordListener currently requires that the given input element not already have a listener, and crashes with an ASSERT failure if it does. Unfortunately the method can be called multiple times on the same elements if the page contains duplicate forms with the same action URL. This causes such pages to crash debug builds of Chromium*.

The fix is either (a) make the caller defensive and check the current list of listeners before trying to add a new one, or (b) make registerPasswordListener treat duplicate listeners as a recoverable error by returning false. Option (a) requires more code and incurs runtime overhead, so I think (b) is best.

Note that since registerPasswordListener's contract is to take ownership of the listener pointer, it must delete it immediately if it can't register it.

* http://code.google.com/p/chromium/issues/detail?id=20418
Comment 1 Jens Alfke 2010-05-07 12:19:47 PDT
Created attachment 55405 [details]
Comment 2 WebKit Review Bot 2010-05-07 12:23:13 PDT
Attachment 55405 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebFrameImpl.cpp:1950:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 4 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Fisher (:fishd, Google) 2010-05-07 12:57:05 PDT
Why not just have registerPasswordListener clobber the existing listener?  Or, provide a hasPasswordListener method?  It might also be nice to support unregistering a listener (perhaps by passing 0 to registerPasswordListener).
Comment 4 Jens Alfke 2010-05-07 13:17:48 PDT
Created attachment 55413 [details]
patch 2

Added a space to appease the coding style gods.
Comment 5 Darin Fisher (:fishd, Google) 2010-05-07 13:48:21 PDT
Comment on attachment 55413 [details]
patch 2

OK, I'm convinced.  It might be nice to provide a way to unregister a listener,
but that doesn't have to be part of this patch, and perhaps we can wait on that
until we have a need for it.

Comment 6 WebKit Commit Bot 2010-05-08 11:06:47 PDT
Comment on attachment 55413 [details]
patch 2

Clearing flags on attachment: 55413

Committed r59029: <http://trac.webkit.org/changeset/59029>
Comment 7 WebKit Commit Bot 2010-05-08 11:06:53 PDT
All reviewed patches have been landed.  Closing bug.