The view's scrollbars send notifications through the ScrollAnimator for things like painting, resizing, etc. Scrollbars for overflow areas need to do the same. <rdar://problem/8944558>
Created attachment 85271 [details] Patch
Comment on attachment 85271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85271&action=review > Source/WebCore/page/FocusController.cpp:416 > + HashSet<ScrollableArea*>::iterator end = scrollableAreas->end(); > + for (HashSet<ScrollableArea*>::iterator it = scrollableAreas->begin(); it != end; ++it) { These could be const_iterators. > Source/WebCore/page/FrameView.cpp:2063 > + HashSet<ScrollableArea*>::iterator end = scrollableAreas->end(); > + for (HashSet<ScrollableArea*>::iterator it = scrollableAreas->begin(); it != end; ++it) Ditto.
(In reply to comment #2) > (From update of attachment 85271 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=85271&action=review > > > Source/WebCore/page/FocusController.cpp:416 > > + HashSet<ScrollableArea*>::iterator end = scrollableAreas->end(); > > + for (HashSet<ScrollableArea*>::iterator it = scrollableAreas->begin(); it != end; ++it) { > > These could be const_iterators. > > > Source/WebCore/page/FrameView.cpp:2063 > > + HashSet<ScrollableArea*>::iterator end = scrollableAreas->end(); > > + for (HashSet<ScrollableArea*>::iterator it = scrollableAreas->begin(); it != end; ++it) > > Ditto. Fixed!
Comment on attachment 85271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85271&action=review > Source/WebCore/page/EventHandler.cpp:1873 > + RenderLayer* layerForLastNode = m_lastNodeUnderMouse && m_lastNodeUnderMouse->renderer() > + ? m_lastNodeUnderMouse->renderer()->enclosingLayer() : 0; > + RenderLayer* layerForNodeUnderMouse = m_nodeUnderMouse && m_nodeUnderMouse->renderer() > + ? m_nodeUnderMouse->renderer()->enclosingLayer() : 0; I’d like to use a helper function to get the layer for a node so we don’t have these unpleasant ? : expressions. That helper could also be used in EventHandler::mouseMoved. > Source/WebCore/page/EventHandler.cpp:1877 > + // The mouse has moved between frames We normally use periods in this kind of comment. > Source/WebCore/page/FocusController.cpp:414 > + HashSet<ScrollableArea*>* scrollableAreas = m_page->scrollableAreaSet(); Missing check for null. > Source/WebCore/page/FrameView.cpp:2061 > + HashSet<ScrollableArea*>* scrollableAreas = page->scrollableAreaSet(); Missing check for null. > Source/WebCore/page/Page.cpp:898 > + m_scrollableAreaSet = new ScrollableAreaSet; Please use adoptPtr here. > Source/WebCore/page/Page.h:286 > + bool pageContainsScrollableArea(ScrollableArea*) const; Don’t need the word page in this function name. The class is Page. > Source/WebCore/page/Page.h:287 > + ScrollableAreaSet* scrollableAreaSet() const { return m_scrollableAreaSet.get(); } This should probably return a const ScrollableAreaSet* since you only use it to iterate all the areas.
Thanks Darin! I will address all of your comments before checking in.
Fixed with revision 80800.
It seems like this patch caused all sorts of crashes on Windows 7 bot: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r80797%20(10268)/results.html http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r80800%20(10269)/results.html blame list: http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/10269
http://trac.webkit.org/changeset/80800 might have broken Qt Linux Release The following tests are not passing: editing/selection/4975120.html editing/selection/caret-and-focus-ring.html fast/dom/Window/window-onFocus.html fast/events/blur-focus-window-should-blur-focus-element.html fast/events/show-modal-dialog-onblur-onfocus.html fast/repaint/fixed-move-after-keyboard-scroll.html plugins/netscape-plugin-setwindow-size-2.html
I rolled it out in http://trac.webkit.org/changeset/80804 for now.
Also caused similar failures on GTK: http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Debug/builds/14301 http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/20246
Looks like it also caused crashes in FocusController::setActive on Leopard: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r80803%20(27453)/results.html
The Windows crash log say a null dereference is happening here: FAULTING_SOURCE_CODE: 2064: return; 2065: 2066: HashSet<ScrollableArea*>::const_iterator end = scrollableAreas->end(); 2067: for (HashSet<ScrollableArea*>::const_iterator it = scrollableAreas->begin(); it != end; ++it) > 2068: (*it)->scrollAnimator()->contentAreaWillPaint(); 2069: } 2070: 2071: #if ENABLE(DASHBOARD_SUPPORT) 2072: void FrameView::updateDashboardRegions() 2073: { I don't know whether *it was null or scrollAnimator() was null.
Created attachment 85488 [details] Better Patch Sorry about the crashes. The crashes happen on MacOS X too, and I was able to find a reproducible case. I discovered that when we crashed, the ScrollableArea was garbage. The problem is that we added the ScrollableArea, and were not able to remove it because when the ScrollableArea was destroyed, we no longer had a reference to the Page. Here is a new patch that adds a member variable to the relevant ScrollableArea classes to keep a weak pointer to Page so that they don't have to rely on Frames and/or Renderers having Pages still. This patch fixes the reproducible case that I found. However, I do not have time to give this patch the testing that it needs today, so I do not intend to check it in until I live on it more. I will update the bug once I have done that. In the meantime, if anyone has comments on the approach or the patch in general, please comment :-)
Attachment 85488 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/page/Page.cpp:198: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Oops, meant to re-open this when I posted the new patch.
Comment on attachment 85488 [details] Better Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85488&action=review Not sure why the patch is not applying on some platforms. >> Source/WebCore/page/Page.cpp:198 >> + } > > One line control clauses should not use braces. [whitespace/braces] [4] I agree with the style bot on this.
Patch committed with revision 81209.
http://trac.webkit.org/changeset/81209 might have broken GTK Linux 32-bit Release
(In reply to comment #13) > Created an attachment (id=85488) [details] > Better Patch > > Sorry about the crashes. The crashes happen on MacOS X too, and I was able to find a reproducible case. I discovered that when we crashed, the ScrollableArea was garbage. The problem is that we added the ScrollableArea, and were not able to remove it because when the ScrollableArea was destroyed, we no longer had a reference to the Page. Here is a new patch that adds a member variable to the relevant ScrollableArea classes to keep a weak pointer to Page so that they don't have to rely on Frames and/or Renderers having Pages still. This patch fixes the reproducible case that I found. > > However, I do not have time to give this patch the testing that it needs today, so I do not intend to check it in until I live on it more. I will update the bug once I have done that. > > In the meantime, if anyone has comments on the approach or the patch in general, please comment :-) m_page seems dangerous and redundant. Is it safe to remove the ScrollableArea through Frame::detachFromPage or FrameLoader::detachFromParent so we can remove m_page?
(In reply to comment #19) > (In reply to comment #13) > > Created an attachment (id=85488) [details] [details] > > Better Patch > > > > Sorry about the crashes. The crashes happen on MacOS X too, and I was able to find a reproducible case. I discovered that when we crashed, the ScrollableArea was garbage. The problem is that we added the ScrollableArea, and were not able to remove it because when the ScrollableArea was destroyed, we no longer had a reference to the Page. Here is a new patch that adds a member variable to the relevant ScrollableArea classes to keep a weak pointer to Page so that they don't have to rely on Frames and/or Renderers having Pages still. This patch fixes the reproducible case that I found. > > > > However, I do not have time to give this patch the testing that it needs today, so I do not intend to check it in until I live on it more. I will update the bug once I have done that. > > > > In the meantime, if anyone has comments on the approach or the patch in general, please comment :-) > > m_page seems dangerous and redundant. Is it safe to remove the ScrollableArea through Frame::detachFromPage or FrameLoader::detachFromParent so we can remove m_page? Beth, we are seeing issues where the cached m_page (named weak, but in fact just a raw point to page) is cause crashes, likely in middle of its destruction or in rare cases. I think Yong's question is pertinent, so we avoid the m_page cache at all. What do you think?