Bug 38765 - [Chromium] WebFrame::registerPasswordListener shouldn't assert on duplicate listener
: [Chromium] WebFrame::registerPasswordListener shouldn't assert on duplicate l...
: WebKit
WebKit API
: 528+ (Nightly build)
: All All
: P3 Minor
Assigned To:
  Show dependency treegraph
Reported: 2010-05-07 12:13 PST by
Modified: 2010-05-08 11:06 PST (History)

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


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

Description From 2010-05-07 12:13:20 PST
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 From 2010-05-07 12:19:47 PST -------
Created an attachment (id=55405) [details]
------- Comment #2 From 2010-05-07 12:23:13 PST -------
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 From 2010-05-07 12:57:05 PST -------
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 From 2010-05-07 13:17:48 PST -------
Created an attachment (id=55413) [details]
patch 2

Added a space to appease the coding style gods.
------- Comment #5 From 2010-05-07 13:48:21 PST -------
(From update of attachment 55413 [details])
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 From 2010-05-08 11:06:47 PST -------
(From update of attachment 55413 [details])
Clearing flags on attachment: 55413

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