Bug 73135

Summary: Remove unnecessary asserts in HTMLTextAreaElement.
Product: WebKit Reporter: John Knottenbelt <jknotten>
Component: New BugsAssignee: John Knottenbelt <jknotten>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, fishd, ojan, rniwa, tkent, tonyg, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

John Knottenbelt
Reported 2011-11-25 07:57:55 PST
[Chromium] Update document style before focusing textarea node.
Attachments
Patch (1.70 KB, patch)
2011-11-25 07:58 PST, John Knottenbelt
no flags
Patch (2.66 KB, patch)
2011-11-28 09:54 PST, John Knottenbelt
no flags
Patch (2.15 KB, patch)
2011-11-30 02:31 PST, John Knottenbelt
no flags
Patch (2.14 KB, patch)
2011-11-30 02:39 PST, John Knottenbelt
no flags
John Knottenbelt
Comment 1 2011-11-25 07:58:50 PST
Eric Seidel (no email)
Comment 2 2011-11-25 09:39:48 PST
Who authored the ASSERT that your'e hitting? Are they CC'd?
Tony Gentilcore
Comment 3 2011-11-25 10:02:00 PST
(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
Comment 4 2011-11-25 11:51:35 PST
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
Comment 5 2011-11-25 11:52:18 PST
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 6 2011-11-27 19:24:37 PST
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
Comment 7 2011-11-28 09:54:01 PST
John Knottenbelt
Comment 8 2011-11-28 09:57:13 PST
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
Comment 9 2011-11-28 19:23:38 PST
(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
Comment 10 2011-11-28 19:53:54 PST
(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
Comment 11 2011-11-29 09:41:17 PST
(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
Comment 12 2011-11-29 12:00:11 PST
(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
Comment 13 2011-11-29 12:16:40 PST
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
Comment 14 2011-11-29 12:18:30 PST
(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
Comment 15 2011-11-30 01:34:25 PST
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
Comment 16 2011-11-30 02:31:49 PST
Kent Tamura
Comment 17 2011-11-30 02:32:41 PST
Comment on attachment 117151 [details] Patch ok
Kent Tamura
Comment 18 2011-11-30 02:34:42 PST
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
Comment 19 2011-11-30 02:39:34 PST
WebKit Review Bot
Comment 20 2011-11-30 07:28:20 PST
Comment on attachment 117152 [details] Patch Clearing flags on attachment: 117152 Committed r101513: <http://trac.webkit.org/changeset/101513>
WebKit Review Bot
Comment 21 2011-11-30 07:28:26 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.