Bug 56067 – Overlay scrollers in overflow areas need send notifications at appropriate times (showing up, resizing)

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

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>

[Beth Dakin]

Created attachment 85271 [details]
Patch

[Simon Fraser (smfr)]

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.

[Beth Dakin]

(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!

[Darin Adler]

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.

[Beth Dakin]

Thanks Darin! I will address all of your comments before checking in.

[Beth Dakin]

Fixed with revision 80800.

[Ryosuke Niwa]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

I rolled it out in http://trac.webkit.org/changeset/80804 for now.

[Ryosuke Niwa]

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

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

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.

[Beth Dakin]

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 :-)

[WebKit Review Bot]

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.

[Beth Dakin]

Oops, meant to re-open this when I posted the new patch.

[Darin Adler]

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.

[Beth Dakin]

Patch committed with revision 81209.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81209 might have broken GTK Linux 32-bit Release

[Yong Li]

(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?

[Antonio Gomes]

(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?