Bug 38765 – [Chromium] WebFrame::registerPasswordListener shouldn't assert on duplicate listener

• Component: WebKit API
• Status: RESOLVED FIXED

[Jens Alfke]

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

[Jens Alfke]

Created attachment 55405 [details]
patch

[WebKit Review Bot]

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.

[Darin Fisher (:fishd, Google)]

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).

[Jens Alfke]

Created attachment 55413 [details]
patch 2

Added a space to appease the coding style gods.

[Darin Fisher (:fishd, Google)]

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.

R=me

[WebKit Commit Bot]

Comment on attachment 55413 [details]
patch 2

Clearing flags on attachment: 55413

Committed r59029: <http://trac.webkit.org/changeset/59029>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.