Bug 73135 – Remove unnecessary asserts in HTMLTextAreaElement.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Knottenbelt]

[Chromium] Update document style before focusing textarea node.

[John Knottenbelt]

Created attachment 116621 [details]
Patch

[Eric Seidel (no email)]

Who authored the ASSERT that your'e hitting?  Are they CC'd?

[Tony Gentilcore]

(In reply to comment #2)
> Who authored the ASSERT that your'e hitting?  Are they CC'd?

Yes. Ojan in http://trac.webkit.org/changeset/46437

[Ryosuke Niwa]

Could you post stack trace or something? I can't make sense of this bug because there's no description whatsoever as to why you're making this change.

[Ryosuke Niwa]

In fact, I'd argue that we shouldn't land this patch until we add some test or the reason for the change is explained in the changelog.

[Kent Tamura]

Comment on attachment 116621 [details]
Patch

> In fact, I'd argue that we shouldn't land this patch until we add some test or the reason for the change is explained in the changelog.

Agree.  ChangeLog should have a reason of the change.

[John Knottenbelt]

Created attachment 116772 [details]
Patch

[John Knottenbelt]

I updated the ChangeLog with the following, which I think bares repeating here:

        http://code.google.com/p/chromium/issues/detail?id=103228 shows that
        sometimes we are hitting the following assert in
        HTMLTextAreaElement::updateFocusAppearance:

        ASSERT(!document()->childNeedsAndNotInStyleRecalc());

        This assert was added by https://bugs.webkit.org/show_bug.cgi?id=27474
        as part of a fix for a crash when the selection is set immediately
        after setting display:none.

        WebViewImpl::setFocus is caused as a result of an IPC from Chrome,
        so if the document can already be requiring a style update, it
        seems reasonable to do it now to satisfy the constraint of the
        HTMLTextAreaElement::updateFocusAppearance assert.

I also discovered Document::updateLayoutIgnorePendingStylesheets which is called from HTMLTextFormControlElement::setSelectionRange, so I'm not sure whether I should be using that instead.

I will try be to adapt Ojan's related test fast/dom/textarea-crash-on-focus.html for this case (coming soon). 

If anybody could comment if I'm on the right or wrong path here, it would be much appreciated.

[Kent Tamura]

(In reply to comment #8)
> I also discovered Document::updateLayoutIgnorePendingStylesheets which is called from HTMLTextFormControlElement::setSelectionRange, so I'm not sure whether I should be using that instead.

Hmm, the ASSERT was needed because updateFocusAppearance() calls setSelectionRange(), however setSeleectionRange() now has a updateLayoutIgnorePendingStylesheets() call.  I guess the ASSERT is not needed anymore.

Ojan, Niwa-san, what do you think?

[Ryosuke Niwa]

(In reply to comment #9)
> (In reply to comment #8)
> > I also discovered Document::updateLayoutIgnorePendingStylesheets which is called from HTMLTextFormControlElement::setSelectionRange, so I'm not sure whether I should be using that instead.
> 
> Hmm, the ASSERT was needed because updateFocusAppearance() calls setSelectionRange(), however setSeleectionRange() now has a updateLayoutIgnorePendingStylesheets() call.  I guess the ASSERT is not needed anymore.
> 
> Ojan, Niwa-san, what do you think?

Yeah, it seems like this assertion is no longer needed.

[Ojan Vafai]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > I also discovered Document::updateLayoutIgnorePendingStylesheets which is called from HTMLTextFormControlElement::setSelectionRange, so I'm not sure whether I should be using that instead.
> > 
> > Hmm, the ASSERT was needed because updateFocusAppearance() calls setSelectionRange(), however setSeleectionRange() now has a updateLayoutIgnorePendingStylesheets() call.  I guess the ASSERT is not needed anymore.
> > 
> > Ojan, Niwa-san, what do you think?
> 
> Yeah, it seems like this assertion is no longer needed.

Yup, also the ASSERT(renderer()) right above it isn't needed either as best I can tell.

Do we see a crash in release builds or is the bug just that we're hitting the assert?

[Ryosuke Niwa]

(In reply to comment #11)
> Do we see a crash in release builds or is the bug just that we're hitting the assert?

See http://code.google.com/p/chromium/issues/detail?id=103228

http://crbu.com/103228

[John Knottenbelt]

Hitting the assert is only an issue with the debug build. However, I did have a look at the crash reports and there are some crashes with WebCore::HTMLTextAreaElement::updateFocusAppearance in the callstack. Most are from version 15.x, but there is 1 for 16.x and 17.x. Since the crashes are for the release build of Chrome, I'm not sure whether they would be related to the asserted condition not holding and causing a problem later. 

(In reply to comment #12)
> (In reply to comment #11)
> > Do we see a crash in release builds or is the bug just that we're hitting the assert?
> 
> See http://code.google.com/p/chromium/issues/detail?id=103228
> 
> http://crbu.com/103228

[Ryosuke Niwa]

(In reply to comment #13)
> Hitting the assert is only an issue with the debug build. However, I did have a look at the crash reports and there are some crashes with WebCore::HTMLTextAreaElement::updateFocusAppearance in the callstack. Most are from version 15.x, but there is 1 for 16.x and 17.x. Since the crashes are for the release build of Chrome, I'm not sure whether they would be related to the asserted condition not holding and causing a problem later. 

With the same stack trace?

[John Knottenbelt]

I had another look, and I couldn't find any crashes with the same stack trace (i.e. with WebViewImpl::setFocus eventually calling HTMLTextAreaElement::updateFocusAppearance ). 

I'll upload a patch to remove the asserts.

(In reply to comment #14)
> (In reply to comment #13)
> > Hitting the assert is only an issue with the debug build. However, I did have a look at the crash reports and there are some crashes with WebCore::HTMLTextAreaElement::updateFocusAppearance in the callstack. Most are from version 15.x, but there is 1 for 16.x and 17.x. Since the crashes are for the release build of Chrome, I'm not sure whether they would be related to the asserted condition not holding and causing a problem later. 
> 
> With the same stack trace?

[John Knottenbelt]

Created attachment 117151 [details]
Patch

[Kent Tamura]

Comment on attachment 117151 [details]
Patch

ok

[Kent Tamura]

Comment on attachment 117151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117151&action=review

> Source/WebCore/ChangeLog:3
> +        [Chromium] Remove unnecessary asserts in HTMLTextAreaElement

This is not a Chromium-specific change.

[John Knottenbelt]

Created attachment 117152 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117152 [details]
Patch

Clearing flags on attachment: 117152

Committed r101513: <http://trac.webkit.org/changeset/101513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.