Bug 26279 – REGRESSION: typing in gmail pegs CPU

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ojan Vafai]

This has been reported by a number of users on the Chrome dev channel and I've seen this personally a number of times in WebKit nightlies.

Typing in rich-text in gmail at some point gets really slow. It's so slow that it uses an entire core on my machine and takes up to a minute or two to catch up to typing a single line of text.

It seems that it might be spellcheck related, but that's just speculation. Hitting enter or clicking to move the cursor elsewhere always seems to fix it.

As far as I know, gmail team hasn't heard reports of this in non-WebKit browsers or in Safari 4/Chrome 2. So this seems likely to be a recent regression from the past week or two.

See the chromium bug for more information: http://bugs.chromium.org/12785

[Ojan Vafai]

CC a bunch of reviewers in the hopes that they have guesses as to what the offending change might be. Unfortunately, we still lack a consistent repro and haven't been able to hit this outside of gmail.

[Eric Seidel (no email)]

Can we get a sample?

I've definitely had Gmail trouble for the last while.  But I figured it was flash trouble.

If it's spellcheck related it could be the disable spellcheck attribute we added?

[Julie Parent]

When it gets in the bad state (incredibly laggy typing), there are several things that cause it to go back to normal, including: hitting enter, clicking into another paragraph, or using arrow keys to navigate to another paragraph.  Typing other characters or just waiting forever never causes it to get better.  This seems like a selection change (into another block?) is what gets it out of the bad state.

Is there any info we can collect when we see it in the bad state? It happens to me at least once a day.  What do you mean by "collect a sample"?

[Eric Seidel (no email)]

On Mac OS X: Instruments, Spin Doctor, "sample" from the command line, "Sample.app" or "Shark".  Any of these will collect samples, which are stack traces showing where we're spending our time. :)

[Ojan Vafai]

Forgive my n00b-dom here, but is there somewhere I can get symbols for Safari 4? I'm on a release build, so getting doesn't tell me much. I guess I can spend the next couple days using a debug build in the hopes of hitting this problem.

[Justin Garcia]

Might be the recent changes for text checking.  Hopefully someone can get a sample.

[Ojan Vafai]

Created attachment 31171 [details]
shark sample at r44610

I think I got a sample of the bad behavior from a debug build. The behavior was slightly different though, so I'm not 100% sure it's the same issue. When typing, it didn't totally freeze up, letters kept popping in very slowly. But it did have the same behavior of getting better when I hit enter and then backspace.

[Eric Seidel (no email)]

Shark sample has line number stripped.  But, ojan's set up to capture it in the debugger next time and we'll investigate from there.

[Eric Seidel (no email)]

Oh, the Shark sample is still useful though!  We're spending all our time applying styles and splitting text nodes.  Not quite sure why we'd be doing that on text insertion, but Justin might know by just looking at the sample.

[Eric Seidel (no email)]

Created attachment 31172 [details]
text version of the hottest callstack for easy viewing

[Justin Garcia]

Hmm any idea what the DOM was like?

[Ojan Vafai]

OK. Here's what I know. 

The problem is that in InsertTextCommand::input we have a typingStyle that actually has a property in it. The property looks to be a font-size:
(gdb) p typingStyle->m_properties[0]
$15 = (const WebCore::CSSProperty &) @0x28097810: {
  m_id = 1046, 
  m_shorthandID = 0, 
  m_important = false, 
  m_implicit = false, 
  m_value = {
    m_ptr = 0x1c1b7870
  }
}

My guess is that the reason that hitting enter or whatever fixes the perf problem is that it clears this phantom typingStyle. FWIW, the lastEditingCommand is a WebCore::TypingCommand::DeleteKey. So, it might be that it got this weird typingStyle when I hit delete. A number of the comments in the Chromium bug mention doing a delete right before hitting the perf bug.

That said, that the lastEditingCommand is a DeleteKey is not itself a bug. That will happen anytime you hit delete and then start typing normal characters. The difference is that the first time you type a character, it should apply the typingStyle, so that next time you type a character, there is a typingStyle, but it's length() is 0.

So, I see two problems:
1. The typingStyle is never getting applied.
2. I never actually changed font-size in gmail, so I have no idea where this typing style came from. I guess gmail could be doing it, but that wouldn't explain why it's never getting applied.

[Ojan Vafai]

Created attachment 31209 [details]
Broken DOM

The output of startPosition.node()->showTreeForThis() called from within InsertTextCommand::input. The obvious issues:
1. There is no font-size anywhere in the HTML. So the typingStyle is never actually getting applied!
2. Each character I type is going into it's own textnode.

[Ojan Vafai]

I have a reproduction. I'm pretty sure this is the gmail hang in that it's clearly a performance bug and it gets fixed by all the same things people have noticed (clicking elsewhere, hitting enter, etc).

Go to the following page:
http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cdiv%20contentEditable%20style%3D%22border%3A%201px%20solid%22%3E%0A%20%20%3Cbr%3E%3Cdiv%3Ed%3C%2Fdiv%3E%0A%3C%2Fdiv%3E%0A&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

1. Set a breakpoint at line 220 of InsertTextCommand.cpp applyStyle(typingStyle). This should only be hit if there is a typingStyle.
2. Click in the first line of the contentEditable div (above the 'd').
3. Do a forward-delete
4. Type any character

At step 4, the breakpoint gets hit and shouldn't. A typingStyle should not have been set. More importantly, it's not getting cleared. To see that type any character again and see that we keep hitting the breakpoint.

I'm pretty sure this bug was introduced with http://trac.webkit.org/changeset/43243. The problem is in CSSComputedStyleDeclaration::copyInheritableProperties we do the following transformation:
    if (int keywordSize = m_node->computedStyle()->fontDescription().keywordSize())
        style->setProperty(CSSPropertyFontSize, cssIdentifierForFontSizeKeyword(keywordSize));

But then in DeleteSelectionCommand::calculateTypingStyleAfterDelete, we call:
    RefPtr<CSSComputedStyleDeclaration> endingStyle = computedStyle(m_endingPosition.node());
    endingStyle->diff(m_typingStyle.get());
    if (!m_typingStyle->length())
        m_typingStyle = 0;

m_typingStyle->length() should be 0, but in practice, the length is 1. There is a font-size property that didn't get removed in endingStyle->diff(m_typingStyle.get()).

At first glance, it looks like the fix should be in CSSStyleDeclaration::diff. When we compare CSS values, we need to treat font-size specially here as well, doing the same transformation that we do in copyInheritableProperties.

[Darin Adler]

Great analysis. Dan, what do you think the right fix is?

[Ojan Vafai]

Also, layout testing this might need a bit of work. We'll have to expose typingStyle to DRT. I think that's actually a reasonable thing to do as it would also allow us to test things like execCommand('bold') on a caret selection and to test that typingStyle actually gets cleared when the cursor is moved, which is essentially the cause of this perf bug.

With that exposed, it should be straightfoward to convert the reproduction above to a layout test.

(In reply to comment #15)
> Great analysis. Dan, what do you think the right fix is?

Ojan’s idea sounds right, except I think the fix should be in a new CSSComputedStyleDeclaration::diff().

I think a test based on the symptoms of single-character text nodes would suffice.

Ojan, I am sorry I didn’t get back to you on IRC today. I have been thinking about this bug and http://trac.webkit.org/changeset/43243 some more, and my take on this is the following: CSSComputedStyleDeclarations serve two roles: one is in the implementation of the getComputedStyle DOM API, and another is in supporting editing. In their latter role (which, conceptually, could be served by a distinct class), they should preserve keyword values of the font-size property. I expect that if they did that (which they currently don’t in diff()), then keyword values would not turn into length values anywhere in the editing machinery.

Is my model conceptually wrong, or are you just worried that it may be hard to get the details right? If it’s the latter, do you think it would help to explicitly give CSSComputedStyleDeclarations knowledge of which of the two roles they were created to serve (either by subclassing or adding a “for editing” flag)?

[Ojan Vafai]

Created attachment 31582 [details]
WebCore:


2009-06-19  Ojan Vafai  <[e-mail]>

        Reviewed by NOBODY (OOPS!).

        Add logic to CSSStyleDeclaration::diff to deal with font-sizes that are
        keyword values. When diff is called on a CSSStyleDeclaration, we check
        the keywordSize to see if font-size matches a keyword value.

        This ensures that when we diff a CSSMutableStyleDeclaration returned from
        copyInheritableProperties on a CSSComputedStyleDeclaration that we
        correctly identify matching font-sizes.

        We should consider having copyInheritableProperties return something
        other than a CSSMutableStyleDeclaration though (e.g. a
        CSSMutableStyleDeclarationForEditing to ensure that this
        sort of font-size mismatch can't happen in the future.

        https://bugs.webkit.org/show_bug.cgi?id=26279

        Test: editing/inserting/font-size-clears-from-typing-style.html

        * css/CSSComputedStyleDeclaration.cpp:
        (WebCore::CSSStyleDeclaration::cssPropertyMatches):
        * css/CSSComputedStyleDeclaration.h:
        * css/CSSStyleDeclaration.cpp:
        (WebCore::CSSStyleDeclaration::cssPropertyMatches):
        (WebCore::CSSStyleDeclaration::diff):
        * css/CSSStyleDeclaration.h:

LayoutTests:

2009-06-19  Ojan Vafai  <[e-mail]>

        Reviewed by NOBODY (OOPS!).

        This test hits an edge case where typingStyle would never get cleared.
        In addition to making every text insertion go into its own text node,
        this caused large performance problems.

        https://bugs.webkit.org/show_bug.cgi?id=26279

        * editing/inserting/font-size-clears-from-typing-style-expected.txt: Added.
        * editing/inserting/font-size-clears-from-typing-style.html: Added.
        * editing/inserting/resources/TEMPLATE.html: Copied from LayoutTests/editing/execCommand/resources/TEMPLATE.html.
        * editing/inserting/resources/font-size-clears-from-typing-style.js: Added.
        (editable.onkeyup):
---
 10 files changed, 138 insertions(+), 2 deletions(-)

[Ojan Vafai]

Ack. Sorry about that huge comment message. Having tooling issues.

Thanks for getting back to me Mitz. The distinction you make between the editing and computedStyle uses of CSSComputedStyleDeclaration clarifies a lot. Splitting the classes up seems reasonable to me. For example, copyInheritableProperties is only used from editing code and the work it does actually doesn't seem appropriate for the computedStyle use-case.

That said, in this patch, for the sake of getting this performance issue fixed, I just did the straightforward thing you suggested initially of adding CSSComputedStyleDeclaration::diff.

Long-term, I'm still concerned about future code that might have a mismatch between copyInheritableProperties and CSSStyleDeclaration::diff. One  simple-ish solution to that would be if CSSMutableStyleDeclaration had a fontSizeWasSetFromKeyWord bit, or something like that. Then we asserted in CSSStyleDeclaration::diff that the fontSizeWasSetFromKeyWord bits of the two declarations matched. How does that sound?

Comment on attachment 31582 [details]
WebCore:

Thanks for the patch, Ojan. I think this is the right fix, but there are several problems with the patch:

> +editable.onkeyup = function() {
> +    // The forward delete leaves the cursor in the "wrapper" div.
> +    // After typing, there should be exactly one textnode in the wrapper div
> +    // with all the typed characters.
> +    shouldBe(String(wrapper.childNodes.length), '1');
> +    shouldBe(String(wrapper.firstChild.nodeType), '3');
> +}
> +
> +if (window.eventSender)
> +  eventSender.keyDown('B');

Can we make the test work in-browser by using execCommand('InsertText', …) and doing the shouldBe() tests afterwards?

> +    if (property->id() == CSSPropertyFontSize && property->value()->isPrimitiveValue() && m_node && m_node->computedStyle()) {

The old code called Document::updateLayoutIgnoringPendingStylesheets() before calling computedStyle(). Why is it safe to not update layout here?

> +        if (int keywordSize = m_node->computedStyle()->fontDescription().keywordSize()) {
> +            int sizeValue = cssIdentifierForFontSizeKeyword(keywordSize);
> +            CSSPrimitiveValue* primitiveValue = (CSSPrimitiveValue*)property->value();

Please use a static_cast.

> +            if (primitiveValue->primitiveType() == CSSPrimitiveValue::CSS_IDENT && primitiveValue->getIdent() == sizeValue)
> +                return true;
> +        }
> +    } else {
> +        return CSSStyleDeclaration::cssPropertyMatches(property);
> +    }

A single-line block should not have braces.

> +    return false;
> +}

The logic above is wrong. If property’s value is a length, then this will always return false, instead of comparing lengths. For example, if you added this to the test:
    editable.style.fontSize = "40px"
it would fail (whereas without the patch it passes). While it is true that currently diff() doesn’t convert lengths to px for comparison, so it would fail to match "40pt" with "53px", we shouldn’t make things worse. In other words, fall back on CSSStyleDeclaration::cssPropertyMatches() whenever property->value() is not an identifier.

> +    virtual bool cssPropertyMatches(const CSSProperty* property) const;

The parameter name should be omitted, since it is implied by the type.

> +    return value && (value->cssText() == property->value()->cssText());

I am not a fan of unnecessary parentheses, but I guess sometimes they improve readability.

> +    virtual bool cssPropertyMatches(const CSSProperty* property) const;

Please omit the argument name.

r- because of the layout update and non-keyword sizes issues.

<rdar://problem/6996223>

[Ojan Vafai]

Created attachment 31725 [details]
Apply Mitz's review comments

 10 files changed, 125 insertions(+), 2 deletions(-)

[Ojan Vafai]

Mitz, very sorry about all the style errors. I'll get used to WebKit style someday. :)

All done except below.

> > +    if (property->id() == CSSPropertyFontSize && property->value()->isPrimitiveValue() && m_node && m_node->computedStyle()) {
> 
> The old code called Document::updateLayoutIgnoringPendingStylesheets() before
> calling computedStyle(). Why is it safe to not update layout here?

Which old code? I don't know if it's safe to call computedStyle here. Really that depends on who is calling diff, right? I was just mimicking copyInheritableProperties. Should we be updating layout in copyInheritableProperties as well?

(In reply to comment #25)
> Mitz, very sorry about all the style errors. I'll get used to WebKit style
> someday. :)
> 
> All done except below.
> 
> > > +    if (property->id() == CSSPropertyFontSize && property->value()->isPrimitiveValue() && m_node && m_node->computedStyle()) {
> > 
> > The old code called Document::updateLayoutIgnoringPendingStylesheets() before
> > calling computedStyle(). Why is it safe to not update layout here?
> 
> Which old code?

I meant the way diff() currently behaves: it calls getPropertyCSSValue() and CSSComputedStyleDeclaration::getPropertyCSSValue() calls getPropertyCSSValue(…, UpdateLayout), which updates layout if needed.

> I don't know if it's safe to call computedStyle here. Really
> that depends on who is calling diff, right? I was just mimicking
> copyInheritableProperties. Should we be updating layout in
> copyInheritableProperties as well?

copyInheritableProperties() calls copyPropertiesInSet(), which also uses getPropertyCSSValue() and therefore updates layout if needed. I think it is risky to change diff()’s behavior in this regard.

[Ojan Vafai]

(In reply to comment #26)
Ah. That makes sense. Made the change. Will post a new patch when the tests finish.

[Ojan Vafai]

Created attachment 31752 [details]
Now with update layout and test rebaselines

 12 files changed, 130 insertions(+), 7 deletions(-)

Comment on attachment 31752 [details]
Now with update layout and test rebaselines

> +++ b/LayoutTests/ChangeLog

Please update the change log to include the updated results.

> +        RefPtr<RenderStyle> style = m_node->computedStyle();

This should just be a RenderStyle*.

r=me

[Ojan Vafai]

Created attachment 31755 [details]
Now with update layout, test rebaselines and correct ChangeLogs.

 12 files changed, 133 insertions(+), 7 deletions(-)

[Ojan Vafai]

Ack. Sorry. I realized my ChangeLog didn't include the two tests with new expected results. That's the only difference in the latest patch. I'll remove the review flag and assume the r+ applies there as well.

Comment on attachment 31755 [details]
Now with update layout, test rebaselines and correct ChangeLogs.

> +        RefPtr<RenderStyle> style = m_node->computedStyle();

You should change this to a plain pointer, since computedStyle() returns a pointer and you don’t do anything that could result in the style being deref()ed. I think you have commit access, so you don’t need to post an updated patch :)

[Ojan Vafai]

http://trac.webkit.org/changeset/45014