[Chromium] Update document style before focusing textarea node.
Created attachment 116621 [details] Patch
Who authored the ASSERT that your'e hitting? Are they CC'd?
(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
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.
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.
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.
Created attachment 116772 [details] Patch
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.
(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?
(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.
(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?
(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
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
(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?
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?
Created attachment 117151 [details] Patch
Comment on attachment 117151 [details] Patch ok
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.
Created attachment 117152 [details] Patch
Comment on attachment 117152 [details] Patch Clearing flags on attachment: 117152 Committed r101513: <http://trac.webkit.org/changeset/101513>
All reviewed patches have been landed. Closing bug.