Rework code that hides characters in password fields to streamline a little
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Jan 2015 16:33:40 +0000 (16:33 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Jan 2015 16:33:40 +0000 (16:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=140035

Reviewed by Sam Weinig.

Source/WebCore:

* editing/InsertIntoTextNodeCommand.cpp:
(WebCore::InsertIntoTextNodeCommand::doApply): Pass the offset after the character
we want to reveal instead of the offset before. This is more future proof if we
ever want to handling surrogate pairs or combining marks, rather than hard
coding the likely incorrect rule of "go back by one code unit". Also got rid of
the isSecure check here, since RenderText can do that check inside the
momentarilyRevealLastTypedCharacter instead.

* rendering/RenderText.cpp: Tweaked the SecureTextTimer class: Marked it final
and made it derive from TimerBase privately. Made the constructor explicit and
made it take a reference rather than a pointer. Use initialization in the class
to set up the variable, and use 0 as the special value instead of -1 since we
now store the offset *after* the character to be revealed.
(WebCore::secureTextTimers): Use this function and NeverDestroyed rather than
a global variable gSecureTextTimers. Also use unique_ptr for the map so we
don't have to delete explicitly any more.
(WebCore::SecureTextTimer::SecureTextTimer): Moved out of the class definition
so the class ia a little easier to read.
(WebCore::SecureTextTimer::restart): Renamed since the function name doesn't
have to describe its argument; the function is only called in one place.
(WebCore::SecureTextTimer::takeOffsetAfterLastTypedCharacter): Changed name
and made this a one-shot that always zeroes the offset.
(WebCore::SecureTextTimer::fired): Moved out of line and tweaked as above.
(WebCore::RenderText::willBeDestroyed): Simplified now that the function
secureTextTimers() always returns a map and we can just remove since the
values in the map are unique_ptr, so take care of deletion.
(WebCore::RenderText::setRenderedText): Tweaked the code that calls secureText
to make the iOS case clearer.
(WebCore::RenderText::secureText): Rewrote the function. New version no
longer relies on a special String::fill function; it's kind of strange that
String had a built in concept of replacing a string with one that has the
same length but all with a masking character. This new approach is cleaner.
I had written a version that handles surrogate pairs and combining marks,
but then instead wrote a comment explaining why that's not needed/helpful.
(WebCore::RenderText::momentarilyRevealLastTypedCharacter): Added a check so
this does nothing if we are not securing the text. Also updated logic so that
this doesn't double hash any more and updated for other changes like using
a reference instead of a pointer.

* rendering/RenderText.h: Removed the unneeded isSecure function and updated
the argument name in momentarilyRevealLastTypedCharacter since it's now the
offset after the character, not before the character.

Source/WTF:

* wtf/text/StringImpl.cpp:
(WTF::StringImpl::fill): Deleted.
* wtf/text/StringImpl.h: Deleted StringImpl::fill.
* wtf/text/WTFString.h:
(WTF::String::fill): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@177860 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/text/StringImpl.cpp
Source/WTF/wtf/text/StringImpl.h
Source/WTF/wtf/text/WTFString.h
Source/WebCore/ChangeLog
Source/WebCore/editing/InsertIntoTextNodeCommand.cpp
Source/WebCore/rendering/RenderText.cpp
Source/WebCore/rendering/RenderText.h

index 523a869eb068677a16cb50b2c738fa375528abbc..7982ca411fe6b27f840e088ac689132d392876e5 100644 (file)
@@ -1,3 +1,16 @@
+2015-01-01  Darin Adler  <darin@apple.com>
+
+        Rework code that hides characters in password fields to streamline a little
+        https://bugs.webkit.org/show_bug.cgi?id=140035
+
+        Reviewed by Sam Weinig.
+
+        * wtf/text/StringImpl.cpp:
+        (WTF::StringImpl::fill): Deleted.
+        * wtf/text/StringImpl.h: Deleted StringImpl::fill.
+        * wtf/text/WTFString.h:
+        (WTF::String::fill): Deleted.
+
 2014-12-26  Dan Bernstein  <mitz@apple.com>
 
         Build fix.
index d04c4adc6e6aab0dcc2ae90cec8a767fa5bbb0ba..28a23b42398f98c035c183996357b8260f7bccc2 100644 (file)
@@ -596,22 +596,6 @@ Ref<StringImpl> StringImpl::upper(const AtomicString& localeIdentifier)
     return newString.releaseNonNull();
 }
 
-Ref<StringImpl> StringImpl::fill(UChar character)
-{
-    if (!(character & ~0x7F)) {
-        LChar* data;
-        auto newImpl = createUninitialized(m_length, data);
-        for (unsigned i = 0; i < m_length; ++i)
-            data[i] = character;
-        return newImpl;
-    }
-    UChar* data;
-    auto newImpl = createUninitialized(m_length, data);
-    for (unsigned i = 0; i < m_length; ++i)
-        data[i] = character;
-    return newImpl;
-}
-
 Ref<StringImpl> StringImpl::foldCase()
 {
     // FIXME: Why doesn't this function have a preflight like the one in StringImpl::lower?
index f26edcb05cdc3b7b4c28e93a634e30bea8ead8e0..b66b0cc6c52f0891ffa08d3c6959ddf017e39cd8 100644 (file)
@@ -639,9 +639,6 @@ public:
     WTF_EXPORT_STRING_API Ref<StringImpl> lower(const AtomicString& localeIdentifier);
     WTF_EXPORT_STRING_API Ref<StringImpl> upper(const AtomicString& localeIdentifier);
 
-    WTF_EXPORT_STRING_API Ref<StringImpl> fill(UChar);
-    // FIXME: Do we need fill(char) or can we just do the right thing if UChar is ASCII?
-
     Ref<StringImpl> foldCase();
 
     Ref<StringImpl> stripWhiteSpace();
index b04843f1e38f477b0278014789dcfe2af4920910..3412222bd90c102b2db67b54e06b02d95072614e 100644 (file)
@@ -306,8 +306,6 @@ public:
         return *this;
     }
 
-    void fill(UChar c) { if (m_impl) m_impl = m_impl->fill(c); }
-
     WTF_EXPORT_STRING_API void truncate(unsigned len);
     WTF_EXPORT_STRING_API void remove(unsigned pos, int len = 1);
 
index eca5a5efe8ab4aec1749110bedac4a7b27485dd6..ff1269ade63b25ef7542ac93049f11a1e16854f3 100644 (file)
@@ -1,3 +1,53 @@
+2015-01-01  Darin Adler  <darin@apple.com>
+
+        Rework code that hides characters in password fields to streamline a little
+        https://bugs.webkit.org/show_bug.cgi?id=140035
+
+        Reviewed by Sam Weinig.
+
+        * editing/InsertIntoTextNodeCommand.cpp:
+        (WebCore::InsertIntoTextNodeCommand::doApply): Pass the offset after the character
+        we want to reveal instead of the offset before. This is more future proof if we
+        ever want to handling surrogate pairs or combining marks, rather than hard
+        coding the likely incorrect rule of "go back by one code unit". Also got rid of
+        the isSecure check here, since RenderText can do that check inside the
+        momentarilyRevealLastTypedCharacter instead.
+
+        * rendering/RenderText.cpp: Tweaked the SecureTextTimer class: Marked it final
+        and made it derive from TimerBase privately. Made the constructor explicit and
+        made it take a reference rather than a pointer. Use initialization in the class
+        to set up the variable, and use 0 as the special value instead of -1 since we
+        now store the offset *after* the character to be revealed.
+        (WebCore::secureTextTimers): Use this function and NeverDestroyed rather than
+        a global variable gSecureTextTimers. Also use unique_ptr for the map so we
+        don't have to delete explicitly any more.
+        (WebCore::SecureTextTimer::SecureTextTimer): Moved out of the class definition
+        so the class ia a little easier to read.
+        (WebCore::SecureTextTimer::restart): Renamed since the function name doesn't
+        have to describe its argument; the function is only called in one place.
+        (WebCore::SecureTextTimer::takeOffsetAfterLastTypedCharacter): Changed name
+        and made this a one-shot that always zeroes the offset.
+        (WebCore::SecureTextTimer::fired): Moved out of line and tweaked as above.
+        (WebCore::RenderText::willBeDestroyed): Simplified now that the function
+        secureTextTimers() always returns a map and we can just remove since the
+        values in the map are unique_ptr, so take care of deletion.
+        (WebCore::RenderText::setRenderedText): Tweaked the code that calls secureText
+        to make the iOS case clearer.
+        (WebCore::RenderText::secureText): Rewrote the function. New version no
+        longer relies on a special String::fill function; it's kind of strange that
+        String had a built in concept of replacing a string with one that has the
+        same length but all with a masking character. This new approach is cleaner.
+        I had written a version that handles surrogate pairs and combining marks,
+        but then instead wrote a comment explaining why that's not needed/helpful.
+        (WebCore::RenderText::momentarilyRevealLastTypedCharacter): Added a check so
+        this does nothing if we are not securing the text. Also updated logic so that
+        this doesn't double hash any more and updated for other changes like using
+        a reference instead of a pointer.
+
+        * rendering/RenderText.h: Removed the unneeded isSecure function and updated
+        the argument name in momentarilyRevealLastTypedCharacter since it's now the
+        offset after the character, not before the character.
+
 2015-01-01  Darin Adler  <darin@apple.com>
 
         Modernize coding style of HTMLTreeBuilder
index f553f93450a052fb474dc7c6ece5cba2c240638c..5394e7e09b182e20c3026808878dcf6664691bf1 100644 (file)
@@ -60,9 +60,8 @@ void InsertIntoTextNodeCommand::doApply()
         return;
 
     if (passwordEchoEnabled) {
-        RenderText* renderText = m_node->renderer();
-        if (renderText && renderText->isSecure())
-            renderText->momentarilyRevealLastTypedCharacter(m_offset + m_text.length() - 1);
+        if (RenderText* renderText = m_node->renderer())
+            renderText->momentarilyRevealLastTypedCharacter(m_offset + m_text.length());
     }
 
     m_node->insertData(m_offset, m_text, IGNORE_EXCEPTION);
index 0b23bf41da99016e006b8fe949eb29b27bd4cea2..9407ad223abe93b709c2802a96cd1ba5647386b9 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * (C) 1999 Lars Knoll (knoll@kde.org)
  * (C) 2000 Dirk Mueller (mueller@kde.org)
- * Copyright (C) 2004, 2005, 2006, 2007, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2007, 2013-2015 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Andrew Wellington (proton@wiretapped.net)
  * Copyright (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *
@@ -75,38 +75,53 @@ struct SameSizeAsRenderText : public RenderObject {
 
 COMPILE_ASSERT(sizeof(RenderText) == sizeof(SameSizeAsRenderText), RenderText_should_stay_small);
 
-class SecureTextTimer;
-typedef HashMap<RenderText*, SecureTextTimer*> SecureTextTimerMap;
-static SecureTextTimerMap* gSecureTextTimers = 0;
-
-class SecureTextTimer : public TimerBase {
+class SecureTextTimer final : private TimerBase {
+    WTF_MAKE_FAST_ALLOCATED;
 public:
-    SecureTextTimer(RenderText* renderText)
-        : m_renderText(renderText)
-        , m_lastTypedCharacterOffset(-1)
-    {
-    }
+    explicit SecureTextTimer(RenderText&);
+    void restart(unsigned offsetAfterLastTypedCharacter);
 
-    void restartWithNewText(unsigned lastTypedCharacterOffset)
-    {
-        m_lastTypedCharacterOffset = lastTypedCharacterOffset;
-        const Settings& settings = m_renderText->frame().settings();
-        startOneShot(settings.passwordEchoDurationInSeconds());
-    }
-    void invalidate() { m_lastTypedCharacterOffset = -1; }
-    unsigned lastTypedCharacterOffset() { return m_lastTypedCharacterOffset; }
+    unsigned takeOffsetAfterLastTypedCharacter();
 
 private:
-    virtual void fired()
-    {
-        ASSERT(gSecureTextTimers->contains(m_renderText));
-        m_renderText->setText(m_renderText->text(), true /* forcing setting text as it may be masked later */);
-    }
-
-    RenderText* m_renderText;
-    int m_lastTypedCharacterOffset;
+    virtual void fired() override;
+    RenderText& m_renderer;
+    unsigned m_offsetAfterLastTypedCharacter { 0 };
 };
 
+typedef HashMap<RenderText*, std::unique_ptr<SecureTextTimer>> SecureTextTimerMap;
+
+static SecureTextTimerMap& secureTextTimers()
+{
+    static NeverDestroyed<SecureTextTimerMap> map;
+    return map.get();
+}
+
+inline SecureTextTimer::SecureTextTimer(RenderText& renderer)
+    : m_renderer(renderer)
+{
+}
+
+inline void SecureTextTimer::restart(unsigned offsetAfterLastTypedCharacter)
+{
+    m_offsetAfterLastTypedCharacter = offsetAfterLastTypedCharacter;
+    startOneShot(m_renderer.frame().settings().passwordEchoDurationInSeconds());
+}
+
+inline unsigned SecureTextTimer::takeOffsetAfterLastTypedCharacter()
+{
+    unsigned offset = m_offsetAfterLastTypedCharacter;
+    m_offsetAfterLastTypedCharacter = 0;
+    return offset;
+}
+
+void SecureTextTimer::fired()
+{
+    ASSERT(secureTextTimers().get(&m_renderer) == this);
+    m_offsetAfterLastTypedCharacter = 0;
+    m_renderer.setText(m_renderer.text(), true /* forcing setting text as it may be masked later */);
+}
+
 static HashMap<const RenderText*, String>& originalTextMap()
 {
     static NeverDestroyed<HashMap<const RenderText*, String>> map;
@@ -265,8 +280,7 @@ void RenderText::removeAndDestroyTextBoxes()
 
 void RenderText::willBeDestroyed()
 {
-    if (SecureTextTimer* secureTextTimer = gSecureTextTimers ? gSecureTextTimers->take(this) : 0)
-        delete secureTextTimer;
+    secureTextTimers().remove(this);
 
     removeAndDestroyTextBoxes();
     RenderObject::willBeDestroyed();
@@ -1044,30 +1058,28 @@ void RenderText::setRenderedText(const String& text)
 
     applyTextTransform(style(), m_text, previousCharacter());
 
-    // We use the same characters here as for list markers.
-    // See the listMarkerText function in RenderListMarker.cpp.
     switch (style().textSecurity()) {
     case TSNONE:
         break;
+#if !PLATFORM(IOS)
+    // We use the same characters here as for list markers.
+    // See the listMarkerText function in RenderListMarker.cpp.
     case TSCIRCLE:
-#if PLATFORM(IOS)
-        secureText(blackCircle);
-#else
         secureText(whiteBullet);
-#endif
         break;
     case TSDISC:
-#if PLATFORM(IOS)
-        secureText(blackCircle);
-#else
         secureText(bullet);
-#endif
         break;
     case TSSQUARE:
-#if PLATFORM(IOS)
-        secureText(blackCircle);
-#else
         secureText(blackSquare);
+        break;
+#else
+    // FIXME: Why this quirk on iOS?
+    case TSCIRCLE:
+    case TSDISC:
+    case TSSQUARE:
+        secureText(blackCircle);
+        break;
 #endif
     }
 
@@ -1085,26 +1097,36 @@ void RenderText::setRenderedText(const String& text)
     }
 }
 
-void RenderText::secureText(UChar mask)
+void RenderText::secureText(UChar maskingCharacter)
 {
-    if (!textLength())
+    // This hides the text by replacing all the characters with the masking character.
+    // Offsets within the hidden text have to match offsets within the original text
+    // to handle things like carets and selection, so this won't work right if any
+    // of the characters are surrogate pairs or combining marks. Thus, this function
+    // does not attempt to handle either of those.
+
+    unsigned length = textLength();
+    if (!length)
         return;
 
-    int lastTypedCharacterOffsetToReveal = -1;
-    String revealedText;
-    SecureTextTimer* secureTextTimer = gSecureTextTimers ? gSecureTextTimers->get(this) : nullptr;
-    if (secureTextTimer && secureTextTimer->isActive()) {
-        lastTypedCharacterOffsetToReveal = secureTextTimer->lastTypedCharacterOffset();
-        if (lastTypedCharacterOffsetToReveal >= 0)
-            revealedText = m_text.substring(lastTypedCharacterOffsetToReveal, 1);
-    }
+    UChar characterToReveal = 0;
+    unsigned revealedCharactersOffset;
 
-    m_text.fill(mask);
-    if (lastTypedCharacterOffsetToReveal >= 0) {
-        m_text.replace(lastTypedCharacterOffsetToReveal, 1, revealedText);
-        // m_text may be updated later before timer fires. We invalidate the lastTypedCharacterOffset to avoid inconsistency.
-        secureTextTimer->invalidate();
+    if (SecureTextTimer* timer = secureTextTimers().get(this)) {
+        // We take the offset out of the timer to make this one-shot. We count on this being called only once.
+        // If it's called a second time we assume the text is different and a character should not be revealed.
+        revealedCharactersOffset = timer->takeOffsetAfterLastTypedCharacter();
+        if (revealedCharactersOffset && revealedCharactersOffset <= length)
+            characterToReveal = m_text[--revealedCharactersOffset];
     }
+
+    UChar* characters;
+    m_text = String::createUninitialized(length, characters);
+
+    for (unsigned i = 0; i < length; ++i)
+        characters[i] = maskingCharacter;
+    if (characterToReveal)
+        characters[revealedCharactersOffset] = characterToReveal;
 }
 
 void RenderText::setText(const String& text, bool force)
@@ -1537,17 +1559,14 @@ bool RenderText::computeCanUseSimpleFontCodePath() const
     return Font::characterRangeCodePath(characters16(), length()) == Font::Simple;
 }
 
-void RenderText::momentarilyRevealLastTypedCharacter(unsigned lastTypedCharacterOffset)
+void RenderText::momentarilyRevealLastTypedCharacter(unsigned offsetAfterLastTypedCharacter)
 {
-    if (!gSecureTextTimers)
-        gSecureTextTimers = new SecureTextTimerMap;
-
-    SecureTextTimer* secureTextTimer = gSecureTextTimers->get(this);
-    if (!secureTextTimer) {
-        secureTextTimer = new SecureTextTimer(this);
-        gSecureTextTimers->add(this, secureTextTimer);
-    }
-    secureTextTimer->restartWithNewText(lastTypedCharacterOffset);
+    if (style().textSecurity() == TSNONE)
+        return;
+    auto& secureTextTimer = secureTextTimers().add(this, nullptr).iterator->value;
+    if (!secureTextTimer)
+        secureTextTimer = std::make_unique<SecureTextTimer>(*this);
+    secureTextTimer->restart(offsetAfterLastTypedCharacter);
 }
 
 StringView RenderText::stringView(int start, int stop) const
index 659b1c1274ff35535bb85bd6a27b001c49c9f1b1..43e40bc6dddbe01674426ab34e22701e5dbbf07a 100644 (file)
@@ -138,8 +138,7 @@ public:
 
     bool containsReversedText() const { return m_containsReversedText; }
 
-    bool isSecure() const { return style().textSecurity() != TSNONE; }
-    void momentarilyRevealLastTypedCharacter(unsigned lastTypedCharacterOffset);
+    void momentarilyRevealLastTypedCharacter(unsigned offsetAfterLastTypedCharacter);
 
     InlineTextBox* findNextInlineTextBox(int offset, int& pos) const { return m_lineBoxes.findNext(offset, pos); }